Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Tests pass even if they fail #2287

Closed
2 tasks
matteovivona opened this issue Aug 12, 2020 · 8 comments
Closed
2 tasks

Tests pass even if they fail #2287

matteovivona opened this issue Aug 12, 2020 · 8 comments
Assignees
Labels
🐛bug issue/pull request that documents/fixes a bug in progress indicates that issue/pull request is currently being worked on LOE - large indicates that the level of effort to complete issue is large
Milestone

Comments

@matteovivona
Copy link
Contributor

matteovivona commented Aug 12, 2020

🐛 Bug Report

  • Tests pass even if they fail
  • The tests fail. Understand why

Reference: https://github.com/HospitalRun/hospitalrun-frontend/runs/974788560?check_suite_focus=true#step:8:138

Cc @fox1t

@matteovivona matteovivona added 🐛bug issue/pull request that documents/fixes a bug help wanted indicates that an issue is open for contributions good first issue indicates an issue is good for a first time contributor labels Aug 12, 2020
@matteovivona matteovivona added this to the v2.0 milestone Aug 12, 2020
@matteovivona matteovivona added LOE - large indicates that the level of effort to complete issue is large and removed good first issue indicates an issue is good for a first time contributor labels Aug 12, 2020
@Tomastomaslol
Copy link
Contributor

Tomastomaslol commented Aug 14, 2020

Hi @tehkapa I took a look at this and it looks like it is caused by asynchronous tests not using the await keyword to make sure that all asynchronous operations are closed when the expectation is fulfilled. If my understanding of the issue is correct the reason it doesn't fail the test is that the expectation is still fulfilled even though operations are still happening in the background.

This can be fixed by going through all tests and making sure all asynchronous operations have closed before moving on to the next test. This would be a fairly big task so it might be worth exploring some other options before deciding how to approach this issue.

@fox1t
Copy link
Member

fox1t commented Aug 14, 2020

@Tomastomaslol nice finding! Would you love to make a PR?

@Tomastomaslol
Copy link
Contributor

Yeah sure. I will take a look at it! 🙂

@Tomastomaslol
Copy link
Contributor

Tomastomaslol commented Aug 29, 2020

The tests that keep generates the warnings were introduced in 41a97ad.

I created a branch that fixes the warnings but I keep having issues with this particular test: https://github.com/HospitalRun/hospitalrun-frontend/blob/master/src/__tests__/patients/search/SearchPatients.test.tsx#L33

Any attempt I tried so far to make sure the test does not have any asynchronous operations running after the test has been run has resulted in failures:

Summary of all failing tests
 FAIL  src/__tests__/patients/search/SearchPatients.test.tsx
  ● Search Patients › should update view patients table search request when patient search input changes

    expect(received).toEqual(expected) // deep equality

    - Expected
    + Received

      Object {
    -   "queryString": "someQueryString",
    +   "queryString": "",
      }

      50 | 
      51 |     const patientsTable = wrapper.find(ViewPatientsTable)
    > 52 |     expect(patientsTable.prop('searchRequest')).toEqual(expectedSearch)
         |                                                 ^
      53 |   })
      54 | })
      55 | 

      at Object.<anonymous> (src/__tests__/patients/search/SearchPatients.test.tsx:52:49)


Example code: https://github.com/HospitalRun/hospitalrun-frontend/compare/master...Tomastomaslol:2287_fix_async_tests?expand=1#diff-596c2918a23833fedf2fd54bae374b88R42

Unfortunately, I'm not sure I understand the context well enough to be able to take a decision on how to proceed and make sure the test is verifying the behaviour correctly and get rid of the warnings. Any pointers or help from @jackcmeyer or anyone else would be greatly appreciated.

@yuktashres
Copy link

Hello, is this still available to work on?

@matteovivona
Copy link
Contributor Author

Absolutely @yuktashres

@matteovivona matteovivona added in progress indicates that issue/pull request is currently being worked on and removed help wanted indicates that an issue is open for contributions labels Nov 20, 2020
@JacobMGEvans
Copy link
Contributor

#2516 Should fix this

@JacobMGEvans
Copy link
Contributor

@jackcmeyer I think this issue should be closed after a few days of reviewing the new testing environment and people have had a chance to mess around with it locally.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐛bug issue/pull request that documents/fixes a bug in progress indicates that issue/pull request is currently being worked on LOE - large indicates that the level of effort to complete issue is large
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants