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 tests for devmode timestamps #275

Closed
wants to merge 2 commits into from
Closed

Add tests for devmode timestamps #275

wants to merge 2 commits into from

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Apr 21, 2023

Adds tests for get/setting timestamp offsets in dev mode. Only confirms that the algod request/response is correct as a sanity check.

Example here: algorand/py-algorand-sdk#468

@algochoi algochoi marked this pull request as ready for review April 21, 2023 16:39
@algochoi algochoi requested review from winder and tzaffi April 21, 2023 17:52
Scenario: Setting timestamp offsets in dev mode
When I set the timestamp offset to be 1234
And I get the timestamp offset
Then the timestamp offset should be 1234
Copy link
Contributor

Choose a reason for hiding this comment

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

Is line 9 purely testing GET /v2/devmode/blocks/offset/ ? If so, maybe it's also worth adding a step that calls GET v2/block/{round} and verifies that the offset had the desired effect? Might be tricky to make it non-flaky though...

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Looks good. Just proposing an additional step (or two)

@winder
Copy link
Contributor

winder commented Apr 24, 2023

I don't think an integration test adds much safety here beyond whats already been done in the algod tests.

What do you think about adding the path/response unit tests and calling it a day?

For example: https://github.com/algorand/algorand-sdk-testing/pull/245/files

@tzaffi
Copy link
Contributor

tzaffi commented Apr 24, 2023

I don't think an integration test adds much safety here beyond whats already been done in the algod tests.

What do you think about adding the path/response unit tests and calling it a day?

For example: https://github.com/algorand/algorand-sdk-testing/pull/245/files

I think some kind of E2E in go-algorand or integration test in one repo ought to exist showing that the devmode timestamps are behaving as expected. Maybe this already exists, but I didn't spot such a test in a brief search. Maybe add this test somewhere in this file?

@algochoi
Copy link
Contributor Author

algochoi commented Apr 24, 2023

Alternatively we could implement a full end to end test for devmode APIs for only one SDK (e.g. Python) under the @devmode integration tag and the rest can be tested with the path unit test?

@algochoi
Copy link
Contributor Author

What do you think about adding the path/response unit tests and calling it a day?

Added path/response tests here: #276

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Based on our previous conversations, I think this can be closed. Let me know if you disagree and we can discuss in person.

@algochoi
Copy link
Contributor Author

Sounds good, I'll close this for now.

@algochoi algochoi closed this Apr 27, 2023
@algochoi algochoi deleted the timestamp-tests branch April 27, 2023 13:58
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.

3 participants