566 bug tests not cleaned up afterwards

[DionSat]

This PR:

Resolves #566

1. Tests not being well encapsulated and dependent

Additional Context (optional):

Some tests are not independent of each other and rely on global variables or tests being run beforehand to pass tests.

Future Steps/PRs Needed to Finish This Work (optional):

1. Need to find a way to streamline catching tests like these before developers commit

Issues needing discussion/feedback (optional):

1. How should the tests that rely on each other running before hand be addressed.

[DionSat]

Forgot to change the package.json command back. Let me make change it back so I dont accidently push it into development.

[DionSat]

I can address the global variables easily with after each. Its just the other tests that are a problem. If it fine for me to change the results then I can fix the rest of them. But I'm going to have to change at least half of the tests. Because it seems like most of them use that toBeCalledTimes() function, which I think is a problem.

[timbot1789]

Like I said I can address the global variables easily with after each. Its just the other tests that are a problem. If it fine for me to change the results then I can fix the rest of them. But I'm going to have to change at least half of the tests. Because it seems like most of them use that toBeCalledTimes() function, which I think is a problem.

At least half is a lot. What you describe seems fine in itself, but I wonder if there's a deeper issue here with how we're using the css-mediaquery library. Maybe we can use the window.resizeTo() method with the react testing library cleanup function instead

[timbot1789]

Also shorter branch names plz

[DionSat]

Sorry about the branch name. I created it off the issue and this is what is defaulted too. I'll try to change it.

[timbot1789]

Sorry about the branch name. I created it off the issue and this is what is defaulted too. I'll try to change it.

Haha it's not that big a deal

[DionSat]

Woops one the tests failed. I really need to get into the habit of running the tests before I push. I did not know about that function window.resizeTo(). I don't like using delete or settings things to null so this would be a safer option. So I would have to change some of the tests then. Then for tests that require a smaller window we can resize the window to fit the test or we could put in the afterEach window.resizeTo(1920, 1080) to just reset the window back each time. That would save me some editing.

[DionSat]

Well actually a better version would use the screen width and height like this window.resizeTo(screen.width, screen.height)

[timbot1789]

Sounds good. I'm playing around with it now though and I'm not getting window.resize() to fire properly. There may be some incantation that needs to be performed to get it to work.

I'm also noticing a lot of tests that aren't really necessary. Like the FormSection resize logic doesn't do anything interesting. We can remove all the resize stuff in FormSection, set padding to always be 16px, and remove the entire test. I'll do some other work around that in a separate PR.

[DionSat]

I have a problem. Apparently window.resizeTo is not implemented.

[timbot1789]

I have a problem. Apparently window.resizeTo is not implemented.

Ah well maybe my suggestion doesn't work.

It seems that JSDOM doesn't implement the 'resizeTo' function on its window object.

[timbot1789]

Better idea. It seems that most of our implementations don't directly access the CSS media queries. They instead access MUI's useMediaQuery. We can mock this hook in our tests and get the same effect we've been getting mocking the window object. You can see an example of what I mean here. That function always returns true (which is sufficient for this page), but we can replace it with whatever logic is needed for the specific test.

[DionSat]

So I'm new to testing and maybe I'm doing something wrong. From what I'm trying to understand from the testing the windows. We are using a useMediaQuery() in the actual file to tell if its a small screen and then render out the appropriate flexbox orientation. I tried mocking useMediaQuery() using the way you suggested in the file like this

vi.mock('@mui/material/useMediaQuery', async (importOriginal) => ({
  ...importOriginal,
  useMediaQuery: vi.fn().mockReturnValue(false),
  default: vi.fn().mockReturnValue(false),
}));

To make useMediaQuery return false and true for smallscreen. But not matter how much I change the values it doesn't work. I'm not sure if this is just a simple fix. But I kind of got tired of this and tried just mocking and implementation of the mediaMatch. This is what I got and it works, tell me if you think this is okay.

    window.matchMedia = vi.fn().mockImplementation(
      query => {
        return {
          matches: false,
          media: query,
          onchange: null,
          addListener: vi.fn(),
          removeListener: vi.fn()
        };
      }
    );

It would eliminate the need for css-mediaquery library from what I can tell since we can just set mediaMatch to be true or false. I don't know. I'll keep trying to mock the useMediaQuery module if you want. But I just cant figure out how to get it to work. It also says in the official MUI doc that you can't you have to polyfill it, so I think that's why we started using css-mediaquery. I dont know if they fixed it yet. Link

[DionSat]

I don't think there is a way to get around the css-mediaqueries at least till they implement it in the jsdom. Because I think that's the only way adjust the queries. Which means that most of the work I did was meaningless and I should of just used more of those. Well I thought about this before, just using the createMatchMedia() we already have to create custom window.matchMedia for each test. I tried looking up other methods since it looks like we were trying to get away from css-mediaqueries but I don't really see one. I'm going to change the tests back and just add createMatchMedia for when the the window.matchMedia needs to be reset. I think that's all we need to worry about fixing the tests for now. I added a createMediaMatch in the app.test.jsx file to reset the window after each test file. Honestly I don't think I can do much for this issue.

[leekahung]

I'm not sure if it'll help, but what if we add some kind of screen reset before each test run with beforeEach and move the window resizing inside the test run?

[DionSat]

Thanks for reminding me, I think I tried this before and couldn't figure it out or got confused that App.test.jsx was applying to all the tests. Okay after messing around I found a way to reset the window after each test. I had to change the vitest.config.js and use the setupFiles: [./File-Path] option to make a file that will run afterEach() on each test file. Link. I think this will be enough for this branch for now. I'll also remove all the unnecessary calls I made in the files. We can also use this file to reset any globals in future we find or use. I have one problem though with the linting. One problem I'm getting this linting error Can we just suppress this error.

[leekahung]

One problem I'm getting this linting error Can we just suppress this error.

I've seen a medium article about disabling the linter on the vite configuration files itself for that import error, so I think we can do that here as well. https://medium.com/@zamin_mirzad/how-to-setup-vite-js-react-js-typescript-vitest-js-6e01b6436c6a

[DionSat]

Okay I removed the unnecessary screen resets and kept the global one I made for the setup file. The PR is ready for review.

[timbot1789]

Looks good! Feel free to merge it whenever you want.