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

Added and unified options for the gulp test command #2989

Merged
merged 1 commit into from Mar 1, 2021
Merged

Conversation

esanzgar
Copy link
Contributor

@esanzgar esanzgar commented Feb 10, 2021

The purpose of this PR is to be able to easily run karma tests in other
browsers without manually editing or making local copies of the karma
configuration.

Additions:

  • yarn gulp test --no-browser to avoid launching the default browser.
    It is up to the developer to run the tests in whatever browser she/he
    chooses by navigating to http://localhost:9876/.

  • yarn gulp test --browser <browser> run the tests in a different
    browser/s instead of the default browser. Chrome launcher comes by
    default with karma. Firefox, Safari and others can be installed
    independently.

Modifications:

  • yarn gulp test-watch has been substituted by yarn gulp test --watch for consistency.

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #2989 (ba78c76) into master (802674c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2989   +/-   ##
=======================================
  Coverage   97.83%   97.83%           
=======================================
  Files         208      208           
  Lines        7758     7758           
  Branches     1722     1722           
=======================================
  Hits         7590     7590           
  Misses        168      168           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 802674c...ba78c76. Read the comment docs.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @esanzgar - I added a couple of thoughts about supplying multiple patterns to grep and a possible alternative to the --browser option which will enable running tests in other browsers without installing additional packages. The --watch change I'm fine with.

gulpfile.js Outdated
@@ -34,13 +34,22 @@ const IMAGES_DIR = 'build/images';
function parseCommandLine() {
commander
.option(
'--grep [pattern]',
'--grep [pattern...]',
'Run only tests where filename matches a pattern'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since pattern is a regex, the way I run tests that match one of several patterns is just --grep A|B where "A" and "B" are patterns to match. What might be helpful here though is to change "a pattern" to "a regex pattern" to clarify that it isn't a minimatch pattern or something like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to this, one annoying issue at the moment is that this help documentation only exists in the code. If you run gulp test --help you get Gulp's help output, not the output for the specific command. It would be nice to fix that somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the help text as you suggested.

I will remove the options to allow multiple --grep options.

Regarding adding our own help documentation:

gulpfile.js Outdated
.option(
'--browser [browsers...]',
'Run test with this browsers (default "ChromeHeadless_Custom")',
['ChromeHeadless_Custom']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately with this option you can only specify ChromeHeadless or Chrome since those are the only launchers installed. What I was going to suggest doing instead is to have an option that doesn't launch any browser automatically but instead just launches Karma and waits for the user to load the Karma server in a browser of their choice. When calling the Karma API, this just requires setting the browsers argument to null.

I'm sure what the best name for such an option would be - --no-launch-chrome ("Don't launch headless Chrome to run the tests. Navigate to http://localhost:9876/ to run the tests.")?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a very good idea to allow developers to run the test in whatever browser they wish. I added the option --no-browser for that.

@esanzgar
Copy link
Contributor Author

esanzgar commented Feb 17, 2021

@robertknight, thank you for reviewing my PR and your comments.

  • I removed additional --grep options
  • I added --no-browser option
  • I kept --browser option, just for convenience

@esanzgar esanzgar force-pushed the karma-options branch 3 times, most recently from 7455f55 to aa7021b Compare February 22, 2021 19:34
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. A useful improvement to our dev tools 👍

gulpfile.js Outdated Show resolved Hide resolved
The purpose of this PR is to be able to easily run karma tests in other
browsers without manually editing the karma configuration or making
local copies of the karma configuration.

Additions:

- `yarn test --no-browser` to avoid launching the default browser.
  It is up to the developer to run the tests in whatever browser she/he
  chooses by navigating to http://localhost:9876/.

- `yarn test --browser <browser>` run the tests in a different
  browser/s instead of the default browser. Chrome launcher comes by
  default with karma. Firefox, Safari and others can be installed
  independently.

Modifications:

- `yarn gulp test-watch` has been substituted by `yarn test --watch` for consistency.
@esanzgar esanzgar merged commit 0c1673b into master Mar 1, 2021
@esanzgar esanzgar deleted the karma-options branch March 1, 2021 15:01
esanzgar added a commit that referenced this pull request Mar 1, 2021
With the recent changes in the gulp test command (#2989), everything
that could be achieved by `make servetest` can be done by options to the
`make test` target.
esanzgar added a commit that referenced this pull request Mar 1, 2021
With the recent changes in the gulp test command (#2989), everything
that could be achieved by `make servetest` can be done by options to the
`make test` target.
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.

None yet

2 participants