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

Make test_experimental_notice more targeted in its assertions #3036

Merged
merged 3 commits into from
May 4, 2023

Conversation

BrynCooke
Copy link
Contributor

Relying on a full snapshot of the log is likely to result in failures either due to environmental reasons or other unrelated changes.

Checklist

Complete the checklist (and note appropriate exceptions) before a final PR is raised.

  • Changes are compatible[^1]
  • Documentation[^2] completed
  • Performance impact assessed and acceptable
  • Tests added and passing[^3]
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

[^1]. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or ask for it to be labeled) as manual test

@github-actions

This comment has been minimized.

…at it won't fail on other output changes.
@BrynCooke BrynCooke force-pushed the bryn/experimental_notice_test_targeted branch from f415952 to 055fe75 Compare May 4, 2023 09:20
@BrynCooke BrynCooke marked this pull request as ready for review May 4, 2023 09:22
@BrynCooke BrynCooke requested a review from SimonSapin May 4, 2023 09:22
Copy link
Contributor

@SimonSapin SimonSapin left a comment

Choose a reason for hiding this comment

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

This log message involves non-trivial formatting so it’s unfortunate to no longer test what the full message looks like, but I suppose that’s not worth the fragility this test has exhibited so far.

With this change you can also replace .kill() with .graceful_shutdown(). I used kill to avoid output differences between platforms because graceful_shutdown() is not graceful on Windows so shutdown-related logs end up missing.

@BrynCooke BrynCooke enabled auto-merge (squash) May 4, 2023 09:35
@BrynCooke BrynCooke merged commit 1a17029 into dev May 4, 2023
@BrynCooke BrynCooke deleted the bryn/experimental_notice_test_targeted branch May 4, 2023 09:51
@abernix abernix mentioned this pull request May 4, 2023
garypen pushed a commit that referenced this pull request May 10, 2023
Relying on a full snapshot of the log is likely to result in failures
either due to environmental reasons or other unrelated changes.

<!-- start metadata -->

**Checklist**

Complete the checklist (and note appropriate exceptions) before a final
PR is raised.

- [ ] Changes are compatible[^1]
- [ ] Documentation[^2] completed
- [ ] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [ ] Integration Tests
    - [ ] Manual Tests

**Exceptions**

*Note any exceptions here*

**Notes**

[^1]. It may be appropriate to bring upcoming changes to the attention
of other (impacted) groups. Please endeavour to do this before seeking
PR approval. The mechanism for doing this will vary considerably, so use
your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding
Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or
ask for it to be labeled) as `manual test`

---------

Co-authored-by: bryn <bryn@apollographql.com>
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