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

Remove unnecessary Sleep calls in tests. #342

Merged
merged 5 commits into from
Sep 3, 2024

Conversation

NullHypothesis
Copy link
Contributor

As far as I can tell, these calls are not necessary and slow down our tests considerably. After removing the Sleep calls, I ran the test multiple times as follows:

go test \
    -race \
    -count=1 \
    -timeout 30s \
    -tags experimental \
    -run ^ExampleTimestampArray$ github.com/TileDB-Inc/TileDB-Go/examples

All tests passed, so we should be fine.

As far as I can tell, these calls are not necessary and slow down our
tests considerably.  After removing the `Sleep` calls, I ran the test
multiple times as follows:

    go test \
        -race \
        -count=1 \
        -timeout 30s \
        -tags experimental \
        -run ^ExampleTimestampArray$ github.com/TileDB-Inc/TileDB-Go/examples

All tests passed, so we should be fine.
Copy link
Contributor

@thetorpedodog thetorpedodog left a comment

Choose a reason for hiding this comment

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

I think we still might want these to make sure these don’t land on the same millisecond (or at least to make it clear that they shouldn’t). Maybe reduce it to something like 5 * Millisecond instead?

@NullHypothesis
Copy link
Contributor Author

I think we still might want these to make sure these don’t land on the same millisecond (or at least to make it clear that they shouldn’t). Maybe reduce it to something like 5 * Millisecond instead?

Right, I followed your suggestion in the follow-up commits.

Copy link
Contributor

@thetorpedodog thetorpedodog left a comment

Choose a reason for hiding this comment

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

one minor suggestion on the comment but looks good!

Comment on lines 244 to 245
// Wait a few milliseconds to ensure that we are not going to overwrite
// data.
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of "we are not going to overwrite the data" (which we want to do), it might be better to say "our writes are in different milliseconds to ensure correct ordering".

@NullHypothesis NullHypothesis merged commit e4f7c23 into master Sep 3, 2024
8 checks passed
@NullHypothesis NullHypothesis deleted the phw/remove-unnecessary-sleeps branch September 3, 2024 17:33
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.

2 participants