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

Bugfix: fixed more Cosmos DB tests #1316

Conversation

paullatzelsperger
Copy link
Member

@paullatzelsperger paullatzelsperger commented May 17, 2022

What this PR changes/adds

fixes more CosmosDB tests, which were not caught by the previous PR #1315

Why it does that

Briefly state why the change was necessary.

Further notes

There seems to be a functional difference between the Cosmos Emulator (which I used for testing locally) and the actual CosmosDB instance: when sorting based on a non-existing field, the cosmos emulator returns an empty list, whereas on the "real" cosmos does not affect the result.

Linked Issue(s)

Closes #1313

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • added relevant details to the changelog? (skip with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@codecov-commenter
Copy link

Codecov Report

Merging #1316 (1a99a78) into main (54dcd36) will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1316      +/-   ##
==========================================
+ Coverage   67.51%   67.64%   +0.12%     
==========================================
  Files         717      712       -5     
  Lines       15756    15645     -111     
  Branches     1051     1037      -14     
==========================================
- Hits        10638    10583      -55     
+ Misses       4635     4588      -47     
+ Partials      483      474       -9     
Impacted Files Coverage Δ
...taspaceconnector/api/control/ClientController.java
...nector/api/control/ControlApiServiceExtension.java
...pi/control/response/NegotiationStatusResponse.java
...api/control/ClientControlCatalogApiController.java
.../control/HttpApiKeyAuthContainerRequestFilter.java

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 54dcd36...1a99a78. Read the comment docs.

@paullatzelsperger paullatzelsperger force-pushed the bugfix/1313_broken_cosmostests_2 branch from 1a99a78 to 0d620dd Compare May 17, 2022 08:01
@paullatzelsperger paullatzelsperger marked this pull request as ready for review May 17, 2022 08:02
@@ -420,7 +420,7 @@ void findAll_verifyFiltering_invalidFilterExpression() {

var query = QuerySpec.Builder.newInstance().filter("something contains other").build();

assertThatThrownBy(() -> store.queryNegotiations(query)).isInstanceOfAny(IllegalArgumentException.class).hasMessage("Cannot build SqlParameter for operator: contains");
assertThatThrownBy(() -> store.queryNegotiations(query)).isInstanceOfAny(IllegalArgumentException.class).hasMessage("Cannot build WHERE clause, reason: unsupported operator contains");
Copy link
Member

Choose a reason for hiding this comment

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

In general I would avoid testing exception messages, especially in integration tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Well to me it seems a good idea, so we ensure the exception we are testing for is not masked by an unrelated one

Copy link
Member Author

Choose a reason for hiding this comment

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

I tend to agree with @algattik, and asserting exception message has already surfaced problems for me on several occasions. I like tests to be as rigid as possible - you can never be too certain :)

@paullatzelsperger paullatzelsperger merged commit 91932a6 into eclipse-edc:main May 18, 2022
@juliapampus juliapampus added this to In progress in Connector via automation May 23, 2022
@juliapampus juliapampus moved this from In progress to Done in Connector May 23, 2022
juliapampus pushed a commit to FraunhoferISST/edc-connector that referenced this pull request May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Connector
  
Done
Development

Successfully merging this pull request may close these issues.

Broken Cosmos DB integration tests
4 participants