Use mongo for CI #196

ml-evs · 2020-02-28T12:01:59Z

This PR adds an environment variable that we can use in the CI to force the use of a real mongo with the default settings. It's a bit of a hack, but I think this is the least intrusive way of doing it, otherwise, the way we handle passing the MongoCollections to the routers would have to be changed.

Since the tests assume that the mongo is ephemeral, we don't ever want a user running the tests locally with mongo turned on, as it would either a) fail, b) require them to delete any existing "optimade" database.

I have run into this problem a couple of times when trying to reuse as much of the test server as possible, in a perfect world I think we would decouple the structures collection from the router (which only needs to be passed general EntryCollection) but I don't think it's worth doing that here.

codecov · 2020-02-28T12:16:24Z

Codecov Report

Merging #196 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #196      +/-   ##
==========================================
+ Coverage   86.29%   86.43%   +0.14%     
==========================================
  Files          39       39              
  Lines        1846     1851       +5     
==========================================
+ Hits         1593     1600       +7     
+ Misses        253      251       -2

Flag	Coverage Δ
#unittests	`86.43% <100%> (+0.14%)`	⬆️

Impacted Files	Coverage Δ
optimade/server/entry_collections.py	`87.31% <100%> (+2.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71348db...31fa810. Read the comment docs.

ltalirz · 2020-02-28T12:22:29Z

I'm probably not up to date here - why do we need a real mongo DB on CI?
Is this PR fixing some issue?

CasperWA · 2020-02-28T12:44:38Z

I'm probably not up to date here - why do we need a real mongo DB on CI?
Is this PR fixing some issue?

This is related to the issue found in #173 for mongomock that doesn't implement some feature of pymongo.

ml-evs · 2020-02-28T12:50:52Z

I should have linked above, the discussion was here:

#173 (comment)

The alternative is skipping the tests that involve HAS ANY as they trigger the mongomock issue that I've fixed, until my PR to mongomock (hopefully) gets merged.

ml-evs · 2020-02-28T12:54:45Z

I think I've convinced myself we should be using a real mongo for the CI anyway, as currently pymongo is not tested at any point in our workflow so we wouldn't catch other potential disagreements between mongomock and pymongo. (you could argue we're now not testing mongomock...)

CasperWA · 2020-02-28T13:00:54Z

I think I've convinced myself we should be using a real mongo for the CI anyway, as currently pymongo is not tested at any point in our workflow so we wouldn't catch other potential disagreements between mongomock and pymongo. (you could argue we're now not testing mongomock...)

Yeah, I think this means we should also consider deploying our index meta-database at Materials Cloud using a real MongoDB, maybe, @ltalirz? But before, we should double-check if a filter is even supposed to be supported by an index meta-database - I actually don't think it is ... so we're home safe :)

ltalirz · 2020-02-28T13:13:49Z

The only comment from my side is that we need to be testing the default setup that users of the package will use.
If that is mongomock, we should test it (I thought the reason we chose this was that it is easier to set up?).

CasperWA · 2020-02-28T13:15:20Z

The only comment from my side is that we need to be testing the default setup that users will use.
If that is mongomock, we should test it (I thought the reason we chose this was that it is easier to set up?).

For the regular server, I would say that it's pymongo, but for the index meta-database, this could be mongomock. If we can test for both that would be best.

CasperWA

Nice and elegant.
This way we can do both pymongo and mongomock tests from the workflow yml file as I understand it, right?

Also, I have a comment that I would like a response to before approving :)

optimade/server/entry_collections.py

ml-evs · 2020-02-28T13:47:07Z

The only comment from my side is that we need to be testing the default setup that users of the package will use.
If that is mongomock, we should test it (I thought the reason we chose this was that it is easier to set up?).

I agree that the default should be tested, the mongomock servers are currently tested by the validator (though in the tests for the validator action itself...).

I don't think mongomock was ever intended to be the "default backend", its meant to be used as a convenience package for, well, testing... see e.g. the description of mongomock:

MongoDB is complex. This library aims at a reasonably complete mock of MongoDB for testing purposes, not a perfect replica. This means some features are not likely to make it in any time soon.

Also, since many corner cases are encountered along the way, our goal is to try and TDD our way into completeness. This means that every time we encounter a missing or broken (incompatible) feature, we write a test for it and fix it. There are probably lots of such issues hiding around lurking, so feel free to open issues and/or pull requests and help the project out!

CasperWA · 2020-02-28T15:33:50Z

I don't think mongomock was ever intended to be the "default backend", its meant to be used as a convenience package for, well, testing... see e.g. the description of mongomock:

MongoDB is complex. This library aims at a reasonably complete mock of MongoDB for testing purposes, not a perfect replica. This means some features are not likely to make it in any time soon.
Also, since many corner cases are encountered along the way, our goal is to try and TDD our way into completeness. This means that every time we encounter a missing or broken (incompatible) feature, we write a test for it and fix it. There are probably lots of such issues hiding around lurking, so feel free to open issues and/or pull requests and help the project out!

Sure thing. But since the index meta-database should be able to be "hosted" even by static files, I think we can allow the use of mongomock for this implementation, and also expect users to do so, or?

ml-evs · 2020-02-28T15:53:14Z

Sure thing. But since the index meta-database should be able to be "hosted" even by static files, I think we can allow the use of mongomock for this implementation, and also expect users to do so, or?

Absolutely fine, provided we test both, which we are!

EDIT: or will be, after this PR ;)

ml-evs · 2020-02-28T16:50:37Z

This is ready to go as far as I'm concerned, feel free to add extra tests with the env var turned off (I'm sure OPTIMADE_CI_FORCE_MONGO=0 py.test would work) if that's desired.

CasperWA

I'll just add some testing with mongomock in the backend as well - probably add in my suggested change below as well, unless you have issues with it or another reason why it should be a broad Exception?

optimade/server/entry_collections.py

ml-evs · 2020-03-02T12:15:29Z

After all this, my mongomock PR was merged :) I think it still makes sense to test both here, though.

CasperWA · 2020-03-02T14:13:50Z

After all this, my mongomock PR was merged :) I think it still makes sense to test both here, though.

Great! And yeah, this was a good catch to make sure we test things properly here.

ml-evs · 2020-03-03T16:58:01Z

While we're in the mood for merging, would be good to get a few of these done...

CasperWA · 2020-03-04T08:25:47Z

I just want to add a printing of whether it is using a real mongo or mock mongo db, so that it's clear in the output and for the testing.

Edit: Done - all is good.

CasperWA

Thanks for the work on this @ml-evs !
Feel free to merge if you're happy with my final addition and the current state.

ml-evs

@CasperWA: unfortunately if you turn turn on io from py.test, you also get e.g. the tracebacks for tests that test for failure (see latest runs!) which mangle the output. Can we disable this please?

.github/workflows/deps_lint.yml

CasperWA · 2020-03-04T10:44:13Z

@CasperWA: unfortunately if you turn turn on io from py.test, you also get e.g. the tracebacks for tests that test for failure (see latest runs!) which mangle the output. Can we disable this please?

I know - Hmm, how would you otherwise like to confirm that we are indeed using the chosen backend?

.github/workflows/deps_eager.yml

ml-evs · 2020-03-04T10:50:36Z

@CasperWA: unfortunately if you turn turn on io from py.test, you also get e.g. the tracebacks for tests that test for failure (see latest runs!) which mangle the output. Can we disable this please?

I know - Hmm, how would you otherwise like to confirm that we are indeed using the chosen backend?

Could potentially emit a warning? Though that will confuse our warnings...

Could write to a file at the start of the tests and then cat the file at the end? A bit convoluted...

CasperWA · 2020-03-04T10:54:11Z

@CasperWA: unfortunately if you turn turn on io from py.test, you also get e.g. the tracebacks for tests that test for failure (see latest runs!) which mangle the output. Can we disable this please?

I know - Hmm, how would you otherwise like to confirm that we are indeed using the chosen backend?

Could potentially emit a warning? Though that will confuse our warnings...

Could potential write to a file at the start of the tests and then cat the file at the end? A bit convoluted...

Found this stackoverflow page for inspiration.
I think it is crucial that we can verify which backend we are actually testing, so I consider this addition blocking for this PR. Is this okay with you?

ml-evs · 2020-03-04T11:08:20Z

I think it is crucial that we can verify which backend we are actually testing, so I consider this addition blocking for this PR. Is this okay with you?

Fine by me, do you want to have a go or shall I?

CasperWA · 2020-03-04T11:16:19Z

I think it is crucial that we can verify which backend we are actually testing, so I consider this addition blocking for this PR. Is this okay with you?

Fine by me, do you want to have a go or shall I?

I can try - since it's my beef :)

ml-evs · 2020-03-04T11:54:16Z

I can try - since it's my beef :)

Thinking about it, just writing out to a file could be quite useful, as you can then write a little script that tests that e.g. mongo was used when we expected it to be, that can run just after pytest? Just a thought!

CasperWA · 2020-03-04T14:49:17Z

I can try - since it's my beef :)

Thinking about it, just writing out to a file could be quite useful, as you can then write a little script that tests that e.g. mongo was used when we expected it to be, that can run just after pytest? Just a thought!

I am simply importing the client from entry_collections.py and checking it's class type versus the environment variable.

CasperWA · 2020-03-04T14:49:50Z

I've also updated our pytest dependency to ~=5.3. Was there a specific reason we kept it on v3?
This caused me to find an error in our tests as well :( reported in #204

ml-evs · 2020-03-04T14:56:38Z

I've also updated our pytest dependency to ~=5.3. Was there a specific reason we kept it on v3?
This caused me to find an error in our tests as well :( reported in #204

Not that I'm aware of.

I am simply importing the client from entry_collections.py and checking it's class type versus the environment variable.

This seems suitably meta, nice job!

Expose Mongo ports and pin version

Print what mongo package is being used. Co-Authored-By: Matthew Evans <me388@cam.ac.uk>

Test the ci_force_mongo fallback (reverted) Probably due to import caching (or similar) this cannot be properly tested. It is now instead left out of coverage.

CasperWA · 2020-03-04T16:51:58Z

@ml-evs I cleaned up the commits a bit. Squashed you initial commits, left the combined commit, and squashed my commits into meaningful separate "feature" additions/corrections. I hope this is okay?
Otherwise, you should force push your local branch, and I'll try to reapply my latest additions/corrections - there weren't a lot, since I had to revert the extra test I added.

@CasperWA

Up to v0.6.0. **New features**: - GitHub Action validator that runs `optimade_validator` for a locally running OPTiMaDe server (#191, @CasperWA, tested by @ml-evs) - Support filter queries for `HAS ALL`, `HAS ANY` and `HAS ONLY` and value lists on MongoDB backends (#173, @ml-evs) Note: `OPERATOR` use in value lists are still _not_ supported. - Debug mode. Start the server in debug mode to enable `debug` log-level in `uvicorn` and get a full Python traceback in the JSON response (#190, @CasperWA) - Add testing of mandatory `filter` queries to `optimade_validator` (#205, @ml-evs) **Updates**: - Allow Cross-Origin requests from anywhere (`*`), i.e., enable CORS for both servers (#194, @CasperWA) - Updates to models (correct misspelling, more transparent model class naming, streamline models with respect to python class inheritance, update field descriptions) (#195, @CasperWA) - API change: Rename `optimade.models.toplevel.py` to `optimade.models.responses.py` (#195, @CasperWA) - Update dependencies to newest versions (as of 02.03.2020) (#202, #196, @CasperWA) - Move imports from `starlette` to `fastapi`, where possible (#202, @ml-evs) - Remove custom middleware to redirect slashed URLs in favor of `starlette` implementation (#202, @ml-evs) - CI tests are now performed with a real MongoDB in the backend. CI tests are also performed with a `mongomock` backend for the tests in `server/test_middleware.py`, `server/test_server_validation.py`, and `server/test_config.py` (#196, @ml-evs, additional testing by @CasperWA ) - Remove `/optimade` from base URLs. This was especially important for the OpenAPI schema (#201, #216, @CasperWA, @ml-evs) - Check integrity of query part of the raw URL using a custom middleware (#209, @CasperWA) - New `optimade/server/exceptions.py` to contain custom `HttpException`s, moved `BadRequest` here (#209, @CasperWA) - Pattern and regex testing for `data.available_api_versions` parts in `/info` endpoint and fix tests for the same (#211, @CasperWA) - Restructure import of test data for regular server (#212, @shyamd) **Bug fixes**: - New retrieval URL for Materials-Consortia's list of providers (#187, @CasperWA) - Skip local `HAS ONLY` tests with a `mongomock` backend, since v3.19.0 does not support these (#206, @ml-evs) - Resource ID's can now contain slashes (`/`) (#183, @ml-evs, @CasperWA) - Remove _valid_ version part of base URL in `meta.query.representation` (#201, @CasperWA)

ml-evs force-pushed the ml-evs/ci_use_mongo branch from 087683a to 65f8fc7 Compare February 28, 2020 12:03

ml-evs marked this pull request as ready for review February 28, 2020 12:16

ml-evs requested review from CasperWA and ltalirz February 28, 2020 12:16

CasperWA reviewed Feb 28, 2020

View reviewed changes

optimade/server/entry_collections.py Outdated Show resolved Hide resolved

ml-evs requested a review from CasperWA February 28, 2020 16:49

CasperWA requested changes Mar 2, 2020

View reviewed changes

optimade/server/entry_collections.py Outdated Show resolved Hide resolved

ml-evs commented Mar 2, 2020

View reviewed changes

optimade/server/entry_collections.py Outdated Show resolved Hide resolved

CasperWA force-pushed the ml-evs/ci_use_mongo branch from 0b5c353 to 73b51ba Compare March 2, 2020 14:20

ml-evs requested a review from CasperWA March 3, 2020 16:57

CasperWA force-pushed the ml-evs/ci_use_mongo branch 2 times, most recently from 854c8bf to 07bc83d Compare March 4, 2020 09:49

CasperWA previously approved these changes Mar 4, 2020

View reviewed changes

ml-evs commented Mar 4, 2020

View reviewed changes

.github/workflows/deps_lint.yml Outdated Show resolved Hide resolved

.github/workflows/deps_lint.yml Outdated Show resolved Hide resolved

ml-evs dismissed CasperWA’s stale review via dc9dbc4 March 4, 2020 10:44

ml-evs commented Mar 4, 2020

View reviewed changes

.github/workflows/deps_eager.yml Outdated Show resolved Hide resolved

ml-evs commented Mar 4, 2020

View reviewed changes

.github/workflows/deps_eager.yml Outdated Show resolved Hide resolved

CasperWA force-pushed the ml-evs/ci_use_mongo branch from c7101d8 to c796fc6 Compare March 4, 2020 16:30

ml-evs and others added 3 commits March 4, 2020 17:46

Use mongo for CI with a env hack

65e9fb7

Expose Mongo ports and pin version

Run some CI tests also for mongomock

e26ca29

Change broad except to specific for ci_force_mongo

fecf2ad

Print what mongo package is being used. Co-Authored-By: Matthew Evans <me388@cam.ac.uk>

CasperWA force-pushed the ml-evs/ci_use_mongo branch from 4a5e2ae to c78cc41 Compare March 4, 2020 16:48

CasperWA added 2 commits March 4, 2020 17:49

Test env-var results in correct MongoClient

879effc

Test the ci_force_mongo fallback (reverted) Probably due to import caching (or similar) this cannot be properly tested. It is now instead left out of coverage.

Update to pytest~=5.3

31fa810

CasperWA force-pushed the ml-evs/ci_use_mongo branch from c78cc41 to 31fa810 Compare March 4, 2020 16:49

CasperWA approved these changes Mar 4, 2020

View reviewed changes

ml-evs merged commit f0fc998 into master Mar 4, 2020

CasperWA deleted the ml-evs/ci_use_mongo branch March 4, 2020 16:55

CasperWA mentioned this pull request Mar 5, 2020

Up to v0.6.0 #210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use mongo for CI #196

Use mongo for CI #196

ml-evs commented Feb 28, 2020 •

edited

codecov bot commented Feb 28, 2020 •

edited

ltalirz commented Feb 28, 2020

CasperWA commented Feb 28, 2020 •

edited

ml-evs commented Feb 28, 2020

ml-evs commented Feb 28, 2020

CasperWA commented Feb 28, 2020

ltalirz commented Feb 28, 2020 •

edited

CasperWA commented Feb 28, 2020

CasperWA left a comment

ml-evs commented Feb 28, 2020

CasperWA commented Feb 28, 2020

ml-evs commented Feb 28, 2020 •

edited

ml-evs commented Feb 28, 2020

CasperWA left a comment

ml-evs commented Mar 2, 2020

CasperWA commented Mar 2, 2020

ml-evs commented Mar 3, 2020

CasperWA commented Mar 4, 2020 •

edited

CasperWA left a comment •

edited

ml-evs left a comment •

edited

CasperWA commented Mar 4, 2020

ml-evs commented Mar 4, 2020 •

edited

CasperWA commented Mar 4, 2020

ml-evs commented Mar 4, 2020

CasperWA commented Mar 4, 2020

ml-evs commented Mar 4, 2020

CasperWA commented Mar 4, 2020

CasperWA commented Mar 4, 2020 •

edited

ml-evs commented Mar 4, 2020

CasperWA commented Mar 4, 2020

Use mongo for CI #196

Use mongo for CI #196

Conversation

ml-evs commented Feb 28, 2020 • edited

codecov bot commented Feb 28, 2020 • edited

Codecov Report

ltalirz commented Feb 28, 2020

CasperWA commented Feb 28, 2020 • edited

ml-evs commented Feb 28, 2020

ml-evs commented Feb 28, 2020

CasperWA commented Feb 28, 2020

ltalirz commented Feb 28, 2020 • edited

CasperWA commented Feb 28, 2020

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Feb 28, 2020

CasperWA commented Feb 28, 2020

ml-evs commented Feb 28, 2020 • edited

ml-evs commented Feb 28, 2020

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Mar 2, 2020

CasperWA commented Mar 2, 2020

ml-evs commented Mar 3, 2020

CasperWA commented Mar 4, 2020 • edited

CasperWA left a comment • edited

Choose a reason for hiding this comment

ml-evs left a comment • edited

Choose a reason for hiding this comment

CasperWA commented Mar 4, 2020

ml-evs commented Mar 4, 2020 • edited

CasperWA commented Mar 4, 2020

ml-evs commented Mar 4, 2020

CasperWA commented Mar 4, 2020

ml-evs commented Mar 4, 2020

CasperWA commented Mar 4, 2020

CasperWA commented Mar 4, 2020 • edited

ml-evs commented Mar 4, 2020

CasperWA commented Mar 4, 2020

ml-evs commented Feb 28, 2020 •

edited

codecov bot commented Feb 28, 2020 •

edited

CasperWA commented Feb 28, 2020 •

edited

ltalirz commented Feb 28, 2020 •

edited

ml-evs commented Feb 28, 2020 •

edited

CasperWA commented Mar 4, 2020 •

edited

CasperWA left a comment •

edited

ml-evs left a comment •

edited

ml-evs commented Mar 4, 2020 •

edited

CasperWA commented Mar 4, 2020 •

edited