Improving unit tests (404s, flaky tests, rejected promises, etc...)

[JasonHowellSlavin]

While working on #631 @Ruchika4 mentioned that it might be a good idea to tackle the 404s in our unit tests. It would make the test output cleaner and easier to read. Is this something we want to tackle going forward and retroactively?

[ryanmparrish]

This can often happen because the mock/html files are copied from the .plain.html references so in the /test/ directory all those sources 404.

Media - these src values are stripped out to prevent 404.

vs

marquee - has paths copied from the .plain.html

Would love to hear any thoughts on how these should be tackled?

[mokimo]

Interesting, didn't see the discussion - I raised a small PR for milo fixing most of the "./media_" issues #667

IMO the issue is that those errors go under, as opposed to actual errors tests which are more obvious, so if a dev isn't necessarily keeping an eye out for it, those quickly pile up.

One approach would be first trying to reduce the current errors by just going through all of them https://github.com/adobecom/milo/actions/runs/4819932054/jobs/8583762128?pr=667 at run the tests and trying to get the test authors to revisit their tests and make small improvements

Then a bit more education on what to look out for when writing tests apart of "1 Test passes ✅" would already alleviate this issue a lot IMO

[JasonHowellSlavin]

Nice PR!

I think that you are right, we just have to go through the errors and replicate what was done in your PR and what was shown in @ryanmparrish's comment.

I am all for improving unit tests as, to my mind, they provide documentation to the code we are writing especially since we are attempting to minimize comments within the code itself. Wouldn't be bad to come up with a standard for the describe and it blocks, etc.

[mokimo]

The issue goes a bit further than just 404s, there are a lot of actual network requests being done. I've rephrased the OP-title a bit.

I've found something very intriguing here which would fail tests if they did real requests

❌ Review Comp > could be initialized
Error: Uncaught Error: External resource fetch is disallowed in unit tests, please find a way to mock! (http://localhost:2000/?wtr-session-id=e9Ft8wfqOKZTnD8nKovPH:174)
at t.onerror (node_modules/@web/test-runner-mocha/dist/autorun.js:1:326417)

This might be a good long-term fix, we could also try to filter any 404s programmatically.

[mokimo]

Running the test suite locally taking in the testRunner changes, it would fail a lot of tests. It might need some work to get the new settings running initially - but taking long term programmtic control over how tests are written, would be a big-time saver IMO

Testing the new settings failing tests that do network requests outside of localhost:
Before Chrome: 97/97 test files | 760 passed, 5 failed, 1 skipped
After Chrome: 97/97 test files | 721 passed, 44 failed, 1 skipped

[honstar]

I second the general approach to prevent tests to send actual requests 👍

[mokimo]

PR logging requests that fetch an external resource: #803 (Not failing them yet, as that would require fixing the broken tests within the scope of that PR)

Scripts (utils.loadScript) are not logged yet, we should definitely create a similar PR for that.

[mokimo]

I've started https://github.com/orgs/adobecom/discussions/814 to document good/bad patterns! Hope that will become a good resource where we can keep patterns and point developers to