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.

Sign up for GitHub

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

Jump to bottom

Create completely new TVars in LoggingSpec #2573

Merged
merged 2 commits into from
Mar 23, 2021
Merged

Create completely new TVars in LoggingSpec #2573

merged 2 commits into from
Mar 23, 2021

Conversation

Anviking
Copy link
Member

Issue Number

#2368

Overview

  • Use different TVars in LoggingSpec.
    • It seems the flakiness is caused by different test runs interfering with each other. Using a new TVar each run should hopefully fix it.

Comments

@Anviking
Use different TVars in LoggingSpec
748b3e5 
It seems the flakiness is caused by different test runs interfering with
each other. Using a new TVar each run should hopefully fix it.
@Anviking Anviking force-pushed the anviking/2368/use-different-tvars branch from 1e1bb58 to 748b3e5 Compare March 23, 2021 09:40
@Anviking Anviking self-assigned this Mar 23, 2021
@Anviking Anviking requested a review from rvl March 23, 2021 09:40
KtorZ
KtorZ approved these changes Mar 23, 2021
View reviewed changes
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

I see nothing wrong with that, that's actually cheap and may help with the flakyness indeed. 👍
Those tests don't run in parallel though 🤔

@Anviking
Copy link
Member Author

Those tests don't run in parallel though

There is this comment talking about this:

-- | Remove any partial logs which have come from the previous property test
-- run. These can occur because once the requests have completed, the property
-- finishes, but the response side may still be finishing writing its log
-- messages.
skipPrevLogs :: [ApiLog] -> [ApiLog]

(and actually that function shouldn't be needed now - removing)

@Anviking
Copy link
Member Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 23, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 50d0d11 into master Mar 23, 2021
@iohk-bors iohk-bors bot deleted the anviking/2368/use-different-tvars branch March 23, 2021 11:16
rvl
rvl reviewed Mar 24, 2021
View reviewed changes
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

What this actually does is start a new HTTP server for each hspec test case. It's not only making a new empty TVar.
That will reduce the flakiness ... why didn't I think of that? 😄
There are also two hspec test cases using QuickCheck properties.
In these, the HTTP server and TVar will be re-used for all QuickCheck examples. So I expect to see flaky test failures happen there, for the same reason as before.
I think the best way to make these tests stable is to remove the use property / monadicIO in this spec.

@Anviking Anviking mentioned this pull request Mar 25, 2021
Merged
1 task
@Anviking
Copy link
Member Author

There are also two hspec test cases using QuickCheck properties.
In these, the HTTP server and TVar will be re-used for all QuickCheck examples. So I expect to see flaky test failures happen there, for the same reason as before.
I think the best way to make these tests stable is to remove the use property / monadicIO in this spec.

@rvl ah, good point.

I think this is a quick but fine workaround: #2580

iohk-bors bot added a commit that referenced this pull request Mar 26, 2021
@iohk-bors @Anviking @rvl
Merge #2580
868cb07 
2580: Use withMaxSuccess 1 in LoggingSpec properties r=Anviking a=Anviking

# Issue Number

#2368 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Use `withMaxSuccess 1` to ensure each `ctx` is only used for one test.


# Comments

- Quick and I believe fine workaround for #2573 (review)

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
iohk-bors bot added a commit that referenced this pull request Mar 26, 2021
@iohk-bors @Anviking @rvl
Merge #2580
7c082fc 
2580: Further prevent flakiness of LoggingSpec properties r=Anviking a=Anviking

# Issue Number

#2368 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Use `withMaxSuccess 1` to ensure each `ctx` is only used for one test.


# Comments

- Quick and I believe fine workaround for #2573 (review)

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
iohk-bors bot added a commit that referenced this pull request Mar 26, 2021
@iohk-bors @Anviking @rvl
Merge #2580
69d9d50 
2580: Further prevent flakiness of LoggingSpec properties r=Anviking a=Anviking

# Issue Number

#2368 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Use `withMaxSuccess 1` to ensure each `ctx` is only used for one test.


# Comments

- Quick and I believe fine workaround for #2573 (review)

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
iohk-bors bot added a commit that referenced this pull request Mar 26, 2021
@iohk-bors @Anviking @rvl
Merge #2580
34f4c9b 
2580: Further prevent flakiness of LoggingSpec properties r=Anviking a=Anviking

# Issue Number

#2368 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Use `withMaxSuccess 1` to ensure each `ctx` is only used for one test.


# Comments

- Quick and I believe fine workaround for #2573 (review)

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
iohk-bors bot added a commit that referenced this pull request Mar 27, 2021
@iohk-bors @Anviking @rvl
Merge #2580
1d9005b 
2580: Further prevent flakiness of LoggingSpec properties r=Anviking a=Anviking

# Issue Number

#2368 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Use `withMaxSuccess 1` to ensure each `ctx` is only used for one test.


# Comments

- Quick and I believe fine workaround for #2573 (review)

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
iohk-bors bot added a commit that referenced this pull request Mar 27, 2021
@iohk-bors @Anviking @rvl
Merge #2580
1cd10cd 
2580: Further prevent flakiness of LoggingSpec properties r=Anviking a=Anviking

# Issue Number

#2368 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Use `withMaxSuccess 1` to ensure each `ctx` is only used for one test.


# Comments

- Quick and I believe fine workaround for #2573 (review)

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
iohk-bors bot added a commit that referenced this pull request Mar 28, 2021
@iohk-bors @Anviking @rvl
Merge #2580
9ee5478 
2580: Further prevent flakiness of LoggingSpec properties r=Anviking a=Anviking

# Issue Number

#2368 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Use `withMaxSuccess 1` to ensure each `ctx` is only used for one test.


# Comments

- Quick and I believe fine workaround for #2573 (review)

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
iohk-bors bot added a commit that referenced this pull request Mar 28, 2021
@iohk-bors @Anviking @rvl
Merge #2580
933139c 
2580: Further prevent flakiness of LoggingSpec properties r=Anviking a=Anviking

# Issue Number

#2368 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Use `withMaxSuccess 1` to ensure each `ctx` is only used for one test.


# Comments

- Quick and I believe fine workaround for #2573 (review)

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvl rvl rvl left review comments

@KtorZ KtorZ KtorZ approved these changes

Assignees

@Anviking Anviking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Anviking @rvl @KtorZ