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

refactor(excel2json): modularise unittests (DEV-3040) #659

Merged
merged 8 commits into from Nov 30, 2023

Conversation

Nora-Olivia-Ammann
Copy link
Collaborator

No description provided.

Copy link

linear bot commented Nov 30, 2023

DEV-3040 modularise test

Currently many different function calls for testing are combined in one function. If a test fails it is harder to understand which test failed.

Before:
The function created the resource section and in the class set up saved it as a json in a temp folder. It then read the json and checked that it was the same as the input from the method.
Saving the json is not something we need to test.
But it significantly complicated this test.
Instead now we just take the output from the function and extract the information. No temp needed.
Here again we created a complex test, for a file that was written, with this changed function call we can test the results without having to read and save a temp file.
@Nora-Olivia-Ammann
Copy link
Collaborator Author

See: 2d3cfa6 for my reasoning

@Nora-Olivia-Ammann
Copy link
Collaborator Author

As discussed with Balduin (#654 (comment))
We plan to change some of these test to a integration test module. The modularisation for the file test_properties is in the PR from above, as tests needed to be changed there anyway. I would like to do the change to integration in a separate PR after both of the PRs are merged, so that we can start cleanly.

Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

This PR produced a pretty gnarly diff, so I did more of a skimming than a proper review - but I didn't see any show stoppers, and I like the general notion of the PR, so I would just trust you on this one

@Nora-Olivia-Ammann
Copy link
Collaborator Author

This PR produced a pretty gnarly diff, so I did more of a skimming than a proper review - but I didn't see any show stoppers, and I like the general notion of the PR, so I would just trust you on this one

Thanks. The only conceptual difference I made is that I got rid of this temp file writing and reading. So if that makes sense to you, the rest is just more pytest and breaking up big functions, but no content change.

@BalduinLandolt
Copy link
Collaborator

This PR produced a pretty gnarly diff, so I did more of a skimming than a proper review - but I didn't see any show stoppers, and I like the general notion of the PR, so I would just trust you on this one

Thanks. The only conceptual difference I made is that I got rid of this temp file writing and reading. So if that makes sense to you, the rest is just more pytest and breaking up big functions, but no content change.

It absolutely does.
You could consider turning test_project.py into pytest too, if you like.

@Nora-Olivia-Ammann
Copy link
Collaborator Author

This PR produced a pretty gnarly diff, so I did more of a skimming than a proper review - but I didn't see any show stoppers, and I like the general notion of the PR, so I would just trust you on this one

Thanks. The only conceptual difference I made is that I got rid of this temp file writing and reading. So if that makes sense to you, the rest is just more pytest and breaking up big functions, but no content change.

It absolutely does. You could consider turning test_project.py into pytest too, if you like.

That is a mish-mash I am not fond of myself. But The reason why it isn't, is because I could not find a simple and easy pytest equivalence for dictEqual at all, so I would have to compare keys and then iterate etc. So I kept the unittest. This is the same in the other files.

@Nora-Olivia-Ammann Nora-Olivia-Ammann merged commit 107b1b0 into main Nov 30, 2023
9 checks passed
@Nora-Olivia-Ammann Nora-Olivia-Ammann deleted the wip/dev-3040-modularise-test branch November 30, 2023 13:47
@daschbot daschbot mentioned this pull request Nov 30, 2023
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.

None yet

2 participants