search

codeforpdx / PASS

PASS project - with HMIS module integration
MIT License
24 stars 24 forks source link

Bug: Tests are not well encapsulated/are not cleaned up after completion #566

Closed milofultz closed 4 months ago

milofultz commented 5 months ago

(Originally accidentally made these in the wrong repo 😅 https://github.com/codeforpdx/codepdx_website/issues/75)

When adding tests, calls to window.matchMedia bleed over to all following tests within a file. Each test should be isolated to itself to ensure that the tests are consistent and not affected by unrelated tests or tests in close proximity.

Reproduction Steps:

  1. Run a test on test/components/NavBar/NavMenu.test.jsx. See that it passes.
  2. In test/components/NavBar/NavMenu.test.jsx, move the test named it('renders contacts and civic profile links below 600px' above it('does not render contacts and civic profile links above 600px'.
  3. Run the test on test/components/NavBar/NavMenu.test.jsx again. See that it fails.

Expected Behavior:

The tests should not change their outcomes based on functions/behavior in unrelated tests.

Actual Behavior:

Functions/behavior in tests affect other unrelated tests.

Possible Solution (optional):

  1. Implement an afterEach in a vitest setups file that handles a resetting of window.matchMedia.
  2. Other tests with complex and/or global modifications should be examined to ensure their behaviors are isolated.
DionSat commented 5 months ago

So I want to try to tackle this problem if I can. I did test out using afterEach for the /test/components/modals/NewMessageModal.test.jsx. The test is the same one you encountered that used the window.matchMedia afterwards. If you placed it above the test above it it would fail. But after using the afterEach and setting window.matchMedai to null it then works. image

DionSat commented 5 months ago

It seems like this test is reused a lot, so most of tests will probably have to include this. But there is probably more global variables I don't know about as well in the other tests. I haven't looked over them all yet.

DionSat commented 5 months ago

Another issue I'm worried about is when people add new tests. If you they make any new global variables that are to be reused in another test and don't reset it in the afterEach(because they forgot or something or didn't know) then we'll have to do this again. So I'm trying to see if there is a better way to keep the test data separate.

milofultz commented 5 months ago

@DionSat that's a good point. Maybe we could leverage a lint rule like no-implicit-globals here? I'm not sure it would fit the bill, but might be worth trying

milofultz commented 5 months ago

Considering that there are valid cases for upper level declarations (e.g. setting variables in a describe to be used in child it calls), I think it might be really hard to find a good way to automate something like this.

milofultz commented 5 months ago

So I want to try to tackle this problem if I can. I did test out using afterEach for the /test/components/modals/NewMessageModal.test.jsx. The test is the same one you encountered that used the window.matchMedia afterwards. If you placed it above the test above it it would fail. But after using the afterEach and setting window.matchMedai to null it then works. image

reading this, I realize we could make this afterEach global, so that window.matchMedia always needs to be set manually before each test. Not sure if this is a good idea, but a thought

milofultz commented 5 months ago

One last thing (sorry for the barrage), we may be able to at least try to catch these issues through using the sequence.shuffle option for the vitest runner.

If your tests will run in random order you will lose this performance improvement, but it may be useful to track tests that accidentally depend on another run previously.

DionSat commented 5 months ago

That might be the best option for now. I think we can try to make afterEach Global and try to keep track of each Global variable and dependency. I would honestly prefer to just not use global data but in some cases its necessary. At least we wont have to add afterEach to all of the tests. I think I see that there's already an global afterEach on test/App.test.jsx. image I think we can use this to reset the dependent data. Also that sequence.shuffle is good to know. So we can catch any other dependent tests in the future. In fact I would emphasize using that option on the automated test as well so that each developer can catch that and add dependent data to global afterEach. I was mainly just worried about having to constantly address this because someone merged in a dependent test.

DionSat commented 5 months ago

Sorry for the late reply. I've noticed that the App.text.jsx only applies to it() tests that are not within a describe block. As for tests within the describe(), for now I have to make their own afterEach function to reset window.matchMedia. For now I'm going to be changing each test and later see if I can simplify it.

timbot1789 commented 5 months ago

One last thing (sorry for the barrage), we may be able to at least try to catch these issues through using the sequence.shuffle option for the vitest runner.

If your tests will run in random order you will lose this performance improvement, but it may be useful to track tests that accidentally depend on another run previously.

I know I'm late to the game, but I figured I'd throw in my 2 cents. I like the idea of random test orders. It definitely improves test reliability. The one hiccup here is test performance. Our tests run quite slowly right now for reasons we haven't been able to figure out fully (fixing this issue will help, but probably won't solve, the issue). Doing things that slow test runs even further may be problematic.

timbot1789 commented 5 months ago

@DionSat I'm curious what work you've done so far on this. Would you be able to push up the branch and make it a draft PR? I'd like to be able to help if I can.

DionSat commented 5 months ago

Yeah I haven't been working on it much lately. Mostly I can't decide how to address some issues. I would appreciate the help and some ideas. There are some tests cases that use global variables like the window.matchMedia that are easy to fix. But there are others I've noticed that rely on other tests like this for example. image The last tests relies on the previous tests using saveSolidDatasetAt. For these I think I would have to change the test expectations or results, but I'm not sure if I should. For Now I'll turn this into a draft PR. Should of done that a long time ago. But I was just indecisive.