Tests/reorganise tests#375

[Reillyhewitson]

This PR will close #375

Description

This PR reorganises the tests into integration tests that require an ICAT stack to be running and unit tests that don't.

Testing Instructions

Add a set up instructions describing how the reviewer should test the code

• Review code
• Check GitHub Actions build
• If icatdb Generator Script Consistency Test CI job fails, is this because of a deliberate change made to the script to change generated data (which isn't actually a problem) or is here an underlying issue with the changes made?
• Review changes to test coverage
• Does this change mean a new patch, minor or major version should be made? If so, does one of the commit messages feature fix:, feat: or BREAKING CHANGE: so a release is automatically made via GitHub Actions upon merge?
• {more steps here}

Agile Board Tracking

Connect to #375

[codecov]

Codecov Report

Base: 96.90% // Head: 96.89% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (8627dbb) compared to base (71d81ac).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 8627dbb differs from pull request most recent head 619d3e4. Consider uploading reports for the commit 619d3e4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #393      +/-   ##
==========================================
- Coverage   96.90%   96.89%   -0.02%     
==========================================
  Files          39       39              
  Lines        3331     3319      -12     
  Branches      313      310       -3     
==========================================
- Hits         3228     3216      -12     
  Misses         76       76              
  Partials       27       27              

Impacted Files	Coverage Δ
...atagateway_api/src/datagateway_api/icat/helpers.py	94.25% <100.00%> (-0.32%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[VKTB]

@Reillyhewitson Thank you for doing this, it's nice to see the unit and integration tests separated.

I just saw this while browsing through the repository and thought I would comment on what I had in mind when I was thinking about splitting the tests a while back. The main reason I wanted to split them was that I only wanted to run the unit tests locally to check if any of my changes are breaking anything and not have to wait for the integration tests to run which also require an ICAT stack with test data. With the unit and integration tests being separate, I also thought about running them as separate jobs in the CI.

Is there a command that can be used to only run the unit tests and another that will only run the integration tests? If so, it would be nice if we can have two different jobs in the CI as opposed to one. This way we will not have to set up an ICAT instance and populate it with dummy data if the unit test job fails.

[Reillyhewitson]

Is there a command that can be used to only run the unit tests and another that will only run the integration tests?

Yes, that would be possible, with pytest you can point it at a directory and it will run all the tests in that directory and any directories below. I'm not so sure that if using that to decide whether or not to run integration testing would be possible with our current actions setup, it might be something that needs to be done with composite actions, but I can look into it. It would also be possible to set up a workflow to setup and run integration tests once unit tests have passed.

@VKTB

[VKTB]

It would also be possible to set up a workflow to setup and run integration tests once unit tests have passed.

I don't think we need to set up a new workflow. We can just rename the existing tests job in the existing ci-build.yml workflow to integration-tests (or something along those lines) since that already sets up an ICAT stack and populates it with dummy data, and then just define a unit-tests job before it that will only run the unit tests.

[MRichards99]

All the changes you've made look good, would you be able to separate the tests as you and Viktor have discussed please? Just put them into two separate jobs (in the existing workflow file) but have them run parallel like all the workflows do currently. I'm not sure I can see a reason where the integration tests need to wait until the unit tests have passed (happy to be persuaded otherwise though?)

[VKTB]

I'm not sure I can see a reason where the integration tests need to wait until the unit tests have passed (happy to be persuaded otherwise though?)

In my opinion, I think that the job that runs the integration tests should only start if the job that runs the unit tests passes so not in parallel. This is because if any of the unit tests fail then the chances of some integration tests failing are high (if not 100%). If we run them in parallel then we will just end up having to wait (+/- 15 minutes) for the integrations test job to run while knowing that it will likely fail.

[MRichards99]

Looks good, the split is great. I've just realised some of the README needs updating to reflect the changes you've made (i.e. the name of the Nox sessions has changed). Could you update Automated Testing & Other Development Helpers (Nox) and Running Tests please?

[VKTB]

I was just looking at the GitHub Action logs to see how long the unit tests take to run and wondered why they take ~1m45s. After looking at it more closely, I realised it is because the unit_tests nox session runs the pip uninstall -y setuptools, pip install setuptools<58.0.0 and poetry install commands. Do you think that we can safely remove the Uninstall setuptools, Install older setuptools and Install dependencies steps in the tests CI job given that the nox session takes care of this? The integration_tests nox session also does the same so the steps that I mentioned above seem redundant to me.

[MRichards99]

@VKTB good spot, I didn't realise that.

@Reillyhewitson please could you remove this steps that Viktor has linked? They are in the tests, generator-script-testing twice

[MRichards99]

As per comment

[Reillyhewitson]

@MRichards99 Nox runs in its own virtual environment which means a main poetry install is still needed for the dummy data. I've moved the poetry install to just before the generator is run.