Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the new message passing system in grunt-contrib-jasmine #64

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

jabbany
Copy link
Contributor

@jabbany jabbany commented May 16, 2019

This PR addresses #63 (I think)

Do not merge this PR until the corresponding PR in gruntjs/grunt-contrib-jasmine#302 is merged and released onto npm. (Update: Upstream has been merged and AFAIK is available in an npm released version. Still possibly blocking on gruntjs/grunt-contrib-jasmine#300)

This PR contains changes that will mesh with the new message passing system if it is introduced in grunt-contrib-jasmine.

Bugs:

@jabbany jabbany marked this pull request as ready for review May 22, 2019 20:55
@jabbany jabbany changed the title [WIP] Use the new message passing system in grunt-contrib-jasmine Use the new message passing system in grunt-contrib-jasmine May 22, 2019
@jabbany
Copy link
Contributor Author

jabbany commented May 22, 2019

PR on the other side has been merged.
Still pending new version in grunt-contrib-jasmine.

@pigranada
Copy link

Hi.

When will this be merged? =grunt-contrib-jasmine has released the version v.2.1.3 for update in Puppeteer.

Thanks

@jabbany
Copy link
Contributor Author

jabbany commented Oct 29, 2019

This patch should be very simple to review, but some tests fail right now (somehow certain tests are skipped only when running all tests) which is suspected to be related to gruntjs/grunt-contrib-jasmine#300 but since that hasn't been merged or closed, I can't test if that actually fixes things.

In the long run though this repo needs major refactoring (or it needs to be deprecated/archived) since the istanbul package has been deprecated in favor of nyc which features major API changes. It is not clear whether this module makes sense in the current grunt testing ecosystem.

@maenu maenu merged commit bd95403 into maenu:master Oct 29, 2019
@maenu
Copy link
Owner

maenu commented Oct 29, 2019

I merged with some additional version adjustments, including a bump. But I am hesitant to publish before all tests pass, i.e. the issues with the failing tests of 'test:outfile', 'test:threshold' when used in sequence with other tests.

In any case, as @jabbany mentioned, nyc should be used instead of the deprecated istanbul. Now I am wondering: The purpose of this project was to run client-side Jasmine tests in a browser environment integrated in the CI and collect coverage information. Maybe we should reevaluate a few things. I am not currently active in JS, but here is my opinion:

  • Is grunt still the way for JS CI? For me yes, many people are using it, it is stable, even jQuery uses it (https://github.com/jquery/jquery/blob/master/Gruntfile.js), so I would argue grunt is here to stay for a while.
  • Is grunt-contrib-jasmine the way to go for Jasmine browser tests? I think so, they moved from the deprecated PhantomJS to the Google-backed pupeteer, which also seems to be here to stay.
  • Is istanbul the way to collect coverage information? Not any more, nyc replaced it and deprecated istanbul.

So, judging from the name of this repo, as it includes istanbul, I think it would be best to create a new repo for using nyc. I have no experience with nyc, but I guess a lot of this project could be reused. I would like to wait until gruntjs/grunt-contrib-jasmine#300 is merged and published, so we could test against it. If that works, I will publish a new version, so that we have a working solution for an istanbul setup.

@jabbany As you seem to be active in this domain, what do you think?

@jabbany
Copy link
Contributor Author

jabbany commented Oct 29, 2019

Actually, I'm not particularly active in JS testing frameworks... I just happen to have a project from years ago that depended on this module to do coverage testing. The move to Puppeteer in grunt-contrib-jasmine kind of borked the coverage part of my CI tests so I've been making sparse PRs across the projects to try and get the old toolchain working again.

At this point though, it might not make sense to go with this kind of toolchain, given that nyc is itself a CLI test+coverage tool. The alternative is to make the coverage first-class, so instead of grunt-contrib-jasmine (unit testing) + grunt-template-jasmine-istanbul (special template to generate coverage), the workflow would be grunt-simple-nyc (seems to be the more maintained grunt+nyc modules) + ??? (unit testing framework).

@pigranada
Copy link

It seems that grunt-template-jasmine-istanbul is already a deprecated project. I will be looking to grunt-simple-nyc if it will fit as a solution for our Jasmine test automation or maybe we can fully move to nyc.

Thank you so much for your inputs and suggestion. Really appreciate it.

Mr0grog added a commit to Mr0grog/loglevel that referenced this pull request Jan 17, 2024
…to v0.6

In order to a) remove a critical code injection vulnerability and b) upgrade browser tests to a recent version of Headless Chrome (instead of PhantomJS), this upgrades grunt-contrib-jasmine to v4.0.0 and grunt-template-jasmine-istanbul to v0.6.0 (via git; this version was never published on NPM). Upgrading the former requires upgrading the latter.

Important notes:

1. This is a little iffy, in that this version of grunt-template-jasmine-istanbul was never published to NPM, supposedly on account of it causing some test failures (unclear whether those failures are problems with the code or with the tests): maenu/grunt-template-jasmine-istanbul#64 (comment)

2. This requires installation via `npm install --legacy-peer-deps` because this version of grunt-template-jasmine-istanbul has a peer dependency on grunt-contrib-jasmine v2.x, but we are now on v4.x. It still works fine, but is a little messy to require this extra flag (this should only affect installs that include dev dependencies, not people who are installing loglevel as a dependency of their project).

The best solution to these is probably to drop grunt-template-jasmine-istanbul altogether -- we aren't really using the coverage information it gathers right now. The next alternative is to fork and vendor it like we did for grunt-template-jasmine-requirejs.
Mr0grog added a commit to Mr0grog/loglevel that referenced this pull request Jan 17, 2024
…to v0.6

In order to a) remove a critical code injection vulnerability and b) upgrade browser tests to a recent version of Headless Chrome (instead of PhantomJS), this upgrades grunt-contrib-jasmine to v4.0.0 and grunt-template-jasmine-istanbul to v0.6.0 (via git; this version was never published on NPM). Upgrading the former requires upgrading the latter.

Important notes:

1. This is a little iffy, in that this version of grunt-template-jasmine-istanbul was never published to NPM, supposedly on account of it causing some test failures (unclear whether those failures are problems with the code or with the tests): maenu/grunt-template-jasmine-istanbul#64 (comment)

2. This requires installation via `npm install --legacy-peer-deps` because this version of grunt-template-jasmine-istanbul has a peer dependency on grunt-contrib-jasmine v2.x, but we are now on v4.x. It still works fine, but is a little messy to require this extra flag (this should only affect installs that include dev dependencies, not people who are installing loglevel as a dependency of their project).

The best solution to these is probably to drop grunt-template-jasmine-istanbul altogether -- we aren't really using the coverage information it gathers right now. The next alternative is to fork and vendor it like we did for grunt-template-jasmine-requirejs.
Mr0grog added a commit to Mr0grog/loglevel that referenced this pull request Jan 17, 2024
…to v0.6

In order to a) remove a critical code injection vulnerability and b) upgrade browser tests to a recent version of Headless Chrome (instead of PhantomJS), this upgrades grunt-contrib-jasmine to v4.0.0 and grunt-template-jasmine-istanbul to v0.6.0 (via git; this version was never published on NPM). Upgrading the former requires upgrading the latter.

Important notes:

1. This is a little iffy, in that this version of grunt-template-jasmine-istanbul was never published to NPM, supposedly on account of it causing some test failures (unclear whether those failures are problems with the code or with the tests): maenu/grunt-template-jasmine-istanbul#64 (comment)

2. This requires installation via `npm install --legacy-peer-deps` because this version of grunt-template-jasmine-istanbul has a peer dependency on grunt-contrib-jasmine v2.x, but we are now on v4.x. It still works fine, but is a little messy to require this extra flag (this should only affect installs that include dev dependencies, not people who are installing loglevel as a dependency of their project).

The best solution to these is probably to drop grunt-template-jasmine-istanbul altogether -- we aren't really using the coverage information it gathers right now. The next alternative is to fork and vendor it like we did for grunt-template-jasmine-requirejs.
Mr0grog added a commit to Mr0grog/loglevel that referenced this pull request Jan 17, 2024
…to v0.6

In order to a) remove a critical code injection vulnerability and b) upgrade browser tests to a recent version of Headless Chrome (instead of PhantomJS), this upgrades grunt-contrib-jasmine to v4.0.0 and grunt-template-jasmine-istanbul to v0.6.0 (via git; this version was never published on NPM). Upgrading the former requires upgrading the latter.

Important notes:

1. This is a little iffy, in that this version of grunt-template-jasmine-istanbul was never published to NPM, supposedly on account of it causing some test failures (unclear whether those failures are problems with the code or with the tests): maenu/grunt-template-jasmine-istanbul#64 (comment)

2. This requires installation via `npm install --legacy-peer-deps` because this version of grunt-template-jasmine-istanbul has a peer dependency on grunt-contrib-jasmine v2.x, but we are now on v4.x. It still works fine, but is a little messy to require this extra flag (this should only affect installs that include dev dependencies, not people who are installing loglevel as a dependency of their project).

The best solution to these is probably to drop grunt-template-jasmine-istanbul altogether -- we aren't really using the coverage information it gathers right now. The next alternative is to fork and vendor it like we did for grunt-template-jasmine-requirejs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants