test: fix e2e tests to cleanup properly

[malcolm-kee]

Cleanup browser/server properly when assertion fails.

• This is a bugfix
• This is a feature
• This is a code refactor
• This is a test update
• This is a docs update
• This is a metadata update

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

resolves #4846

[alexander-akait]

So the problem in puppeteer itself? Because if we put just throw new Error("test"); our tests failed succefully (I think I am right here 😄 )

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (99f66cb) 91.96% compared to head (f0adeb0) 91.96%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4847   +/-   ##
=======================================
  Coverage   91.96%   91.96%           
=======================================
  Files          16       16           
  Lines        1704     1704           
  Branches      647      647           
=======================================
  Hits         1567     1567           
  Misses        126      126           
  Partials       11       11           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[malcolm-kee]

@alexander-akait sorry what do you mean? "Failed successfully" need some translation 😆

[alexander-akait]

Failed successfully" need some translation laughing

It means if we will put throw new Error("test"): in our tests (inside it(...)), our tests will be failed, and it is succefully (and expected), because it should happen when we do it 😄

[alexander-akait]

But I see intresting error:

TypeError: Converting circular structure to JSON
        --> starting at object with constructor 'BrowserContext'
        |     property '_browser' -> object with constructor 'Browser'
        --- property '_defaultContext' closes the circle
        at stringify (<anonymous>)

maybe we forgot to cleanup something between runs?

[malcolm-kee]

Failed successfully" need some translation laughing

It means if we will put throw new Error("test"): in our tests (inside it(...)), our tests will be failed, and it is succefully (and expected), because it should happen when we do it 😄

It's not reliable behavior. At least in my machine (M1 Mac) it stucks when that happen, so I think it's better to cleanup properly.

[malcolm-kee]

@alexander-akait I think this is an example of job get stuck in macOS: https://github.com/webpack/webpack-dev-server/actions/runs/4899010293/jobs/8748653004?pr=4813#logs

[alexander-akait]

It's not reliable behavior. At least in my machine (M1 Mac) it stucks when that happen, so I think it's better to cleanup properly.

I mean if you put throw Error("test") in it, will your tests freeze too or is it failed immediately?

[malcolm-kee]

@alexander-akait it will stuck because of dev server.

Minimal test case:

const server = new Server({ port }, webpack(config));

await server.start();

throw new Error("What");

await server.stop();

[alexander-akait]

@malcolm-kee Did you look why it happens? I am fine with test refactor, just want to check it out, maybe we don't close/cleanup something

[malcolm-kee]

@alexander-akait I think it's expected (unless this deviates what you understand).

From my experience and also the docs I refer, in general testing NodeJS server with jest do not call the server's listen method because it will start a long-running process. However, .listen is called in Server.start().

[alexander-akait]

/cc @snitin315 Will it create problems with the next branch rebase, because if no I will merge, if yes, we can emrge it in the next branch

[snitin315]

@alexander-akait Yes, let's merge directly in the next branch. @malcolm-kee Can you rebase the PR on top of the next branch?

[alexander-akait]

@malcolm-kee We remove some things from the next version, so tests doesn't exists, like webpack v4, some API and etc, hope rebasing will be easy, anyway feel free to ask any question

[malcolm-kee]

Hmm too much conflict, I'll close this PR and redo it based on next.

[malcolm-kee]

Replace with #4859