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

few tests for MongoStorage frontend #542

Merged
merged 11 commits into from Oct 12, 2021
Merged

few tests for MongoStorage frontend #542

merged 11 commits into from Oct 12, 2021

Conversation

JoranAngevaare
Copy link
Member

@JoranAngevaare JoranAngevaare commented Oct 7, 2021

What is the problem / what does the code in this PR do
Long outstanding issue that code was untested for the MongoFrontend

Can you briefly describe how it works?
Add a test using an empty mongo server from github actions

Result
~95% coverage on the Mongo-storage Frontend/Backend/Saver
afbeelding

Possible issues.
This uses the action secrets which are not available on branches. This may lead to drops in coverage when the test is not run for a PR from a fork that is not AxFoundation (the preferred method of running).

A possible solution would be to spin up mongodb directly
resolved

Why not use mongomock

  • Good question, to be honest I first set this up in a way that it run fine and now I don't feel like changing it as it also requires changes in the init of the MongoFrontend which I rather don't do if it's not needed.
  • Loading the mongo database takes only ~10 s, so speed is not a concern

* test with clean mdb

* start the test

* start test

* whoops

* maybe too much

* Update test_mongo_frontend.py

* Update test_mongo_frontend.py

* update docs
@JoranAngevaare JoranAngevaare marked this pull request as ready for review October 7, 2021 12:16
@JoranAngevaare JoranAngevaare added the Testing Works on testing code label Oct 7, 2021
@@ -8,6 +8,7 @@ flake8==3.9.2
hypothesis==6.17.4
immutabledict==2.2.0
lz4==3.1.3
mongomock==3.23.0
Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually used for the CMT test, but not listed in the requirements. Let's pin it

Copy link
Contributor

@jmosbacher jmosbacher left a comment

Choose a reason for hiding this comment

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

Nice test, also very useful to have a working example for testing MongoDB dependent code in the test suite.
Is it actually necessary to have local_sf alongside the mongo_sf? i guess being able to compare the two improves the tests but on the downside, if I break something in the DataDirectory related code during development this test will also fail adding a bit less clear what I broke. Im not sure which approach is better, just wanted to raise it before this is merged and forgotten...

@JoranAngevaare
Copy link
Member Author

Nice test, also very useful to have a working example for testing MongoDB dependent code in the test suite. Is it actually necessary to have local_sf alongside the mongo_sf? i guess being able to compare the two improves the tests but on the downside, if I break something in the DataDirectory related code during development this test will also fail adding a bit less clear what I broke. Im not sure which approach is better, just wanted to raise it before this is merged and forgotten...

Very good point actually, I've just refactored it a bit, casting it in different test that should fail for different errors without being affected by changes in the DataDirectory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing Works on testing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants