"Check the browser console for details" but process closes before I can look at the browser

alexandercerutti commented 2 years ago

Hello, I was trying to set up JBR to perform some tests in a package of mine (not published yet). I received this message in the console:

Suite error: undefined
  Message:
    Uncaught Error: An error occurred while loading /__spec__/Tokenizer.spec.mjs. Check the browser console for details.
  Stack:
    Error: An error occurred while loading /__spec__/Tokenizer.spec.mjs. Check the browser console for details.
        at HTMLScriptElement.<anonymous> (http://localhost:53230/__support__/loadEsModule.js:20:11)

So my tests were failing for some reason and I wasn't able to understand which was it because the browser's window was closing even before I could look at it.

So, through the usage of Node inspector, I tried to dig it down in JBR's code and edit it to keep the browser open. I found that webdriver.close() was being called no matter what (in finally at index.js#L116). So I tried to tweak the code and I came up with the solution in Pull Request.

I added a new config.browser flag manualClose which, if set to true, aims to not stop the server so things can be debugged on the new browser window (I also was able to set breakpoints in the console and reload the testing page).
To terminate the browser window, I set up an event to be called only once beforeExit ('cause it seems it can be called multiple times, but the browser window can be closed only once).

Better solutions might be available anyway.

Things to note:

Wasn't able to perform SAUCE Tests (I don't even know what sauce is);
Wasn't able to run successful exit tests due to missing jasmine.debugLog (don't actually know the reason), so once disabled the two rows containing this instruction, all the tests were completed successfully;
If the browser gets closed, without closing Node's process first, the process remains active. I wanted this not to happen but seems like selenium doesn't have an event to notify when the browser completes and/or gets closed;
"Allow edits by maintainers" is enabled on this PR, so you can do any change you want;

Hope this is appreciated. Let me know!

alexandercerutti commented 2 years ago

Oh, tests failed due to the missing variable webdriver in the test I changed, but I wasn't alerted when I executed tests locally. I'm not quite sure what should be put in there instead 🤔

https://github.com/alexandercerutti/jasmine-browser-runner/blob/manual-close/spec/indexSpec.js#L421

sgravrock commented 2 years ago

I appreciate the effort that you put into this.

I assume you were using the runSpecs subcommand. In general, I recommend using the serve subcommand and navigating to the spec runner page by hand when you want to do any interactive debugging. That way you have full control over the lifetime of both the browser window and the server. Assuming that that solves your problem, I don't think I want to add this kind of complexity to the cleanup process. As you've discovered yourself, it's difficult to get right even if we can assume that only one Server is created per process.

If serve doesn't solve the problem for you, I'd appreciate it if you could tell me more about how to reproduce it.

Some notes about the problems you mentioned running into while working on this PR:

Wasn't able to perform SAUCE Tests (I don't even know what sauce is);

That's fine. Saucelabs is a service that we use to run tests on a variety of acutal browsers in our CI builds. We don't expect contributors to have access to it.

Wasn't able to run successful exit tests due to missing jasmine.debugLog (don't actually know the reason), so once disabled the two rows containing this instruction, all the tests were completed successfully;

jasmine.debugLog is a new feature introduced in jasmine-core 4.0.0. Is there any chance you have an older version of jasmine-core installed? That could happen if you'd cloned your repo and run npm install before the 1.0.0 release and then pulled changes in from upstream. If that's the problem, you should be able to fix it by running npm install again.

Oh, tests failed due to the missing variable webdriver in the test I changed, but I wasn't alerted when I executed tests locally. I'm not quite sure what should be put in there instead 🤔

In this case I would have stuck with the pre-existing definition of webdriver. BTW, that error came from eslint, which runs only after a successful test run. The jasmine.debugLog error you mentioned above probably prevented eslint from running locally.

I hope this helps.

alexandercerutti commented 2 years ago

Hey @sgravrock, thank you for your reply! Hope you understand that, since I've opened this PR about two weeks ago, I had to move on to another solution (made of several systems interacting together), because I was stuck (but I could retry with jasmine-browser-runner).

Yes, I was using runSpecs because I wanted to "run test iteratively" (as website says) and I wanted a jasmine-for-node-like experience, with a final report on CLI (I never tested front-end applications, so I'm pretty new to this)... but now that I achieved a result, I must acknowledge I ended up setting up a serve-like system with several other packages. 😂

Now I actually understand that serve might be the best choice when testing browser-running apps, but maybe its explanation in the Readme could be improved to make the differences between runSpecs and serve clearer and more noticeable.

Anyway, I don't remember exactly all the details about the issue, but the main issue was that I was experimenting with ESM modules, so I didn't remember that they require the file extension so imported files couldn't be found. I initially thought my main file was wrong or there were other issues. I guess, anyway, the error can always be replicated by making a script to invoke the error listener you inject through selenium, which makes the error in the first message being reported to the console.

About jasmine.debugLog, I don't know. I'm looking right now at jasmine-core in JBR node_modules and I'm using v4.0.0. I can find the method under lib/jasmine-core/jasmine.js. Maybe it doesn't get exposed for some reason? I didn't clone the repo before having the need of doing this PR (so, two weeks ago).

FYI, the PR has the check "Allow edits from maintainers" active, so if you think some changes should be done before applying, you are free to apply them. 😄

sgravrock commented 2 years ago

FYI, the PR has the check "Allow edits from maintainers" active, so if you think some changes should be done before applying, you are free to apply them. 😄

I appreciate it, but I think what's needed here is a documentation fix rather than a new feature. I've logged #16 for that. Closing this in favor of that issue.

jasmine / jasmine-browser-runner

"Check the browser console for details" but process closes before I can look at the browser #14