Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use mock module and move tests #47

Merged
merged 9 commits into from
Nov 1, 2022

Conversation

DominicOram
Copy link
Contributor

Fixes #45

To test:

  • Confirm tests still pass despite mock and test being moved out of top level

@joeshannon
Copy link
Contributor

Ideally the tests would be separate from the main module(s) so that they wouldn't be present in the application.
Something like a new src directory containing the diffcalc module (and the other modules) and then leaving the tests where they are?
It looks like it would be more work as would have to check the structure still works and might have other implications for development (installing package in dev mode etc.)
Also the GDA diffcalc ScriptProject configuration would be updated.

Due to these complications maybe it is easier to move the tests as you have done but I might ask for a second opinion.

I think it might also be good to wait until we can get the CI running again (#46) to verify the tests can still all run as expected.

@joeshannon
Copy link
Contributor

@isikhar do you have any objections to this being merged?
We can look at making a nicer/more standard solution later possibly but might require more involved changes to GDA.

I would still like to check that we can still run the tests before merging.

@joeshannon
Copy link
Contributor

We should merge #46 first, then fix-up this so that the tests pass.

@DominicOram
Copy link
Contributor Author

@joeshannon and @isikhar, this ok?

Copy link
Contributor

@joeshannon joeshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I think Irakli was previously happy with these changes.

@isikhar
Copy link
Contributor

isikhar commented Sep 29, 2022

These changes are fine with me as well.

@DominicOram
Copy link
Contributor Author

Is the CI known to be broken?

@joeshannon
Copy link
Contributor

No I don't think so. Not sure why it's not running. I see that it run for your fork.

I'm not hugely familiar with GH actions, we currently have

name: Diffcalc build

on:
  push:
  pull_request:

Does that seem correct?

@DominicOram
Copy link
Contributor Author

So it did (see https://github.com/DominicOram/diffcalc/actions/runs/3338486618/jobs/5526204223). Yes, those actions seem correct to me. Are we happy to chalk it up to GH weirdness and get I'll merge this?

@joeshannon joeshannon merged commit 627b472 into DiamondLightSource:master Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move mock and test out of top level
3 participants