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

Add integration tests #53

Conversation

camille-bouvy-frequenz
Copy link
Collaborator

This PR adds integration test for the API client, service, as well as handel. These tests are performed by creating, updating & deleting orders on the simulation database.

As discussed in the latest team meeting, these tests were originally written with the goal of automatising everything. As a consequence, some of them might not be the most coherent: for instance test_list_public_trades() just verifies that the returned list isn’t null. It doesn’t check for the correctness of the output at all. These tests might thus have to be split into system tests (that would be performed manually), and integration test. 

Additionally, I think that having these tests run by the CI is not the most appropriate (especially given that they depend on other components than just the client).

So please suggest alternatives & improvements, & let's make a next level robust, efficient and awesome test suite!😎

@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests labels Sep 26, 2024
@github-actions github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Oct 2, 2024
@camille-bouvy-frequenz camille-bouvy-frequenz force-pushed the integration-test branch 4 times, most recently from 13d0389 to 5e73bef Compare October 2, 2024 10:24
tests/test_api.py Outdated Show resolved Hide resolved
@camille-bouvy-frequenz camille-bouvy-frequenz force-pushed the integration-test branch 2 times, most recently from 5421e88 to 7694f94 Compare October 2, 2024 10:49
@camille-bouvy-frequenz
Copy link
Collaborator Author

camille-bouvy-frequenz commented Oct 2, 2024

As mentioned in slack, the tests are currently not passing because the API_KEY seems like it's not recognised. It's however in the Github secrets, and I'm not sure what the problem is.

@camille-bouvy-frequenz camille-bouvy-frequenz force-pushed the integration-test branch 2 times, most recently from 7716920 to d301ab8 Compare October 7, 2024 08:42
Copy link
Contributor

@matthias-wende-frequenz matthias-wende-frequenz left a comment

Choose a reason for hiding this comment

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

One tiny comment. I'll continue my review tomorrow.

tests/test_api.py Outdated Show resolved Hide resolved
Copy link
Contributor

@matthias-wende-frequenz matthias-wende-frequenz left a comment

Choose a reason for hiding this comment

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

Just one comment. Otherwise this looks good to me!

tests/test_api.py Outdated Show resolved Hide resolved
Copy link

@nhatcher-frequenz nhatcher-frequenz left a comment

Choose a reason for hiding this comment

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

Hi @camille-bouvy-frequenz ,

This is impressive! This is going to make us feel a lot safer of the whole product.
My most important thought:

  • I don't think we should run these tests in the CI just yet. Maybe in the future a subset of these tests can run automatically. The reason is that we can run the test in parallel. So two PRs will write each others tests. Running those manually is not a huge deal because you can understand what's going on and run then again. So basically we are adding a step we don't really have so far, QA. Some human being needs to smoke tests that everything works as expected after each deployment (or right before the release)

  • I think it would be a good idea to "mark" the order and trades we create on each test run or even test case. An idea is to use the tag, but we might use something else.

I think we can merge this PR with these tests and start adding test cases!
Many, many thanks!

tests/test_api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@camille-bouvy-frequenz
Copy link
Collaborator Author

As discussed in the office I created a new workflow for these integration tests instead of having them ran by the CI. They will now be ran every time there is a new release, and can be ran manually from Github (by going into Actions --> Electricity-Trading-API-Integration-Tests --> Run workflow).

@camille-bouvy-frequenz
Copy link
Collaborator Author

With regards to the tags I tried adding tag="api-integration-test" but I get an error from the service @nhatcher-frequenz :

FAILED integration_tests/test_api.py::test_list_gridpool_trades - grpc.aio._call.AioRpcError: <AioRpcError of RPC that terminated with:
        status = StatusCode.ABORTED
        details = "Failed to start transaction. Error: Query Error: error returned from database: duplicate key value violates unique constraint "tag_unique_index""
        debug_error_string = "UNKNOWN:Error received from peer  {created_time:"2024-10-16T17:31:24.654862+02:00", grpc_status:10, grpc_message:"Failed to start transaction. Error: Query Error: error returned from database: duplicate key value violates unique constraint \"tag_unique_index\""}"
>

@camille-bouvy-frequenz
Copy link
Collaborator Author

Fixed the tag now that the changes were made in the service.

@nhatcher-frequenz
Copy link

With regards to the tags I tried adding tag="api-integration-test" but I get an error from the service

Forgot to mention but this should be fixed now.

Signed-off-by: camille-bouvy-frequenz <[email protected]>
Signed-off-by: camille-bouvy-frequenz <[email protected]>
Signed-off-by: camille-bouvy-frequenz <[email protected]>
Signed-off-by: camille-bouvy-frequenz <[email protected]>
Copy link

@nhatcher-frequenz nhatcher-frequenz left a comment

Choose a reason for hiding this comment

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

LGTM!


try:
# Stream trades with a 15-second timeout to avoid indefinite hanging
streamed_order = await asyncio.wait_for(anext(stream), timeout=15)

Choose a reason for hiding this comment

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

I didn't know about anext, my Python is getting Rusty.

@camille-bouvy-frequenz camille-bouvy-frequenz added this pull request to the merge queue Nov 12, 2024
Merged via the queue into frequenz-floss:v0.x.x with commit b9d9985 Nov 12, 2024
14 checks passed
@camille-bouvy-frequenz camille-bouvy-frequenz deleted the integration-test branch November 12, 2024 08:48
@camille-bouvy-frequenz camille-bouvy-frequenz added this to the v1.0.0 milestone Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants