Prevent dev browser from running client tests when headless browser is simulaneously

[0o-de-lally]

If I'm running in TEST_WATCH mode, while developing, my client tests are running in duplicate. I'd prefer the tests to only run in nightmare/phantomjs for consistency ( and be isolated in another process). This also affects any web reporter that eventually may be installed.

I guess there are two options:

1. Have a window global variable set for the headless browsers, and the client tests wait for it.

2. Or use a URL with query string, and have the client check the document href. There could be unexpected interactions with routers.

Either way it seems to require changes here and in dispatch:mocha

[aldeed]

If you mean that they're running in the browser where you're developing, then that is probably a Meteor issue. Meteor runs the client tests automatically on startup when you load the app in any browser.

[0o-de-lally]

Yes that's what I'm referring to. I'm wondering if we could have a variable on both dispatch:mocha and aldeed:browser-tests which effectively skips the tests if your are not in the nightmare/phantomjs environment.

[aldeed]

Should be possible. I'd just want to think carefully about whether there is any reason someone would want them also running in the browser. Since this would be an issue affecting all driver packages, it potentially should have a solution within the Meteor codebase. The relevant file is here: https://github.com/meteor/meteor/blob/devel/packages/meteor/test_environment.js

One simple backward compatible solution would be for the driver package to export a boolean shouldSkipTests in addition to the runTests function, and then we can check that in test_environment.js. @tmeasday might have an opinion.

[0o-de-lally]

True the patch should belong in meteor. Until then do you think it's workable to have an ENV variable to suppress running client tests outside of the intended headless browser? I can propose something if you think it's worthwhile.

[tmeasday]

@aldeed if the driver is deciding whether to export shouldSkipTests = true | false couldn't it just set runTests to a null function?

[tmeasday]

Or just runTests = () { if (process.env.WHATEVER) return false; ... }?

[aldeed]

@tmeasday Yes it could. I guess I was thinking there would be some benefit to standardizing behavior rather than every driver package implementing this slightly differently.

In general this use case isn't documented in the guide. The guide suggests running unit tests on a different port while developing, but I can't think of any reason why it wouldn't be OK to run app-tests while developing off the same process. If we add some guidance in the guide for the expected way to do this, then it will be easier for test driver packages to do what is expected. For example, in app test mode, maybe runTests should not be automatically run on startup by Meteor. Maybe the driver should have the flexibility to run tests automatically on CI but let the user call it manually when not in CI, with a button somewhere in their app or an extension.

[0o-de-lally]

@aldeed, another edge case related to this: running acceptance tests is not possible as-is. If you write an acceptance test (nightmare on server test suites), running it will open a browser window, which in turn will trigger client tests. IMO client tests should only run when specifically called, not on any browser request.

[aldeed]

@keyscores Do you mean *.app-tests.* acceptance tests that you run with meteor test --full-app? Why wouldn't you want those tests to kick off immediately?

If you have some other custom script running acceptance tests in nightmare, then you would be using normal meteor run to start the app and this package wouldn't be involved.

Technically client tests DO only run when specifically called. It's just that Meteor calls them immediately upon page load. We could get around that if we need to.

[0o-de-lally]

Regarding acceptance tests, yes currently I'm running nightmare in parallel to a normal `meteor run`. But it would be convenient to run it in the same process, on the server tests. Regardless I think we should solve the issue of meteor triggering client tests on every browser start. I vote for using a localstorage variable. I don't foresee MDG making changes in the short term to meteor tests.

[aldeed]

@keyscores ok, this isn't a development process I'm familiar with so maybe that's why I'm not understanding the reasoning. Is this what you're trying to do:

1. Run locally with meteor test --full-app in watch mode instead of meteor run
2. Make changes, having acceptance/integration tests rerun on the server when it changes, but only rerun on the client when you click a button?

Normally if I wanted tests to rerun while I'm developing, I would kick off a meteor run in one process and meteor test and/or meteor test --full-app with watch mode in separate processes, running on different ports.

I agree it might be handy to have meteor run flags like --unit-tests and --app-tests which would additionally rerun those tests so that you don't have to start up multiple processes. We'd have to work with MDG on that. I think ideally tests would be served on a separate port automatically.

In the short term we can have client tests not run on startup, but I'm still not sure how you envision someone starting them.

[0o-de-lally]

The electron/phantomJS can start the tests on startup, other browsers not. Let me try to clarify: 1. Run locally with meteor test --full-app in watch mode instead of meteor run 2. Make changes, having acceptance/integration tests rerun on the server AND ELECTRON when it changes. Never run in the dev's browser. What I'm proposing is a replacement of practicalmeteor's interface. The UI is to give feedback as to the test state, and run/rerun tests/suites. Using the command line args is workable (now that we have grep), but it's very tedious for large apps with long rebuilds. I have a working version of this, but I'm getting duplicate tests for the client since it runs in electron and browser (the dev is using a browser to control the test suite). So, importantly, we can't have tests running in the dev's browser if it's also running in electron (I see no use case for running client tests in the dev's browser). All tests should be run in the target environment (server in the meteor process, and client in the electron environment).

[aldeed]

@keyscores For that scenario, I guess I don't know why you don't just run meteor test --full-app --port=3010 and meteor run at the same time in two separate windows.

When you say "other browsers not", what about the web reporter that we are adding? I'm just trying to figure out exactly how the client code would know when to run and when not to. Obviously we can kick it off manually for the headless options, but how does the user start the tests running after they load the page in a browser? In my experience it's much more common that you open the page in the browser and you DO want the tests to run.

I like your vision of combining the run and test commands more, but I'm worried that making it work well this way will break things for people with a different expectation.

[0o-de-lally]

@aldeed. This proposal only applies to testing using headless, so it shouldn't impact non-headless, (direct testing).

If we want the client tests to run in a controlled environment, usually Electron or Phantom, then having client tests also run in the dev's browser (Firefox, Opera, whatever) is unintended. As I see it, we don't have any control at the moment as to how and when the client tests run; they always run on startup.

what about the web reporter that we are adding?

In my design, headless-only mode, the webreporter should only report test results which ran in the controlled environment.

why you don't just run meteor test --full-app --port=3010 and meteor run at the same time in two separate windows.

The problem is still the same. The test reporter will show duplicate test results for client tests.
If the dev still wants to run two meteor instances on different ports that's fine, but would be redundant with the change proposed.