Skip to content

Local error golden test functions #1236

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

newhoggy
Copy link
Contributor

Changelog

- description: |
    Import from `cardano-api` into local functions:
    * `testAllErrorMessages`
    * `testAllErrorMessages_`
    *  `testErrorMessage`
    Call `watchdogProp` when running golden error tests
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@newhoggy newhoggy marked this pull request as ready for review June 29, 2025 13:12
-Wno-x-partial
-Wno-name-shadowing
-Wno-incomplete-uni-patterns

Copy link
Contributor

@erikd erikd Jun 30, 2025

Choose a reason for hiding this comment

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

Why is this needed? Would be nice to have a comment explaining why.

Copy link
Contributor Author

@newhoggy newhoggy Jun 30, 2025

Choose a reason for hiding this comment

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

One of the Windows builds fails otherwise. The asn1-encoding package fails to build because it generates warnings on Windows and our build errors out on warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added.

@newhoggy newhoggy force-pushed the newhoggy/local-error-golden-test-functions branch from 4d6d095 to 987346b Compare June 30, 2025 11:02
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Why are you moving the functions here?

carbolymer

This comment was marked as duplicate.

Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

I have the same question as @Jimbo4350 . Why vendor in functions here, rather than fixing them upstream?

I'm not convinced this PR is a net benefit because I haven't seen hangups in golden tests in cardano-api and we are using those functions there as well.

@newhoggy Have you seen hangups in error messages' goldens? Have you seen them in cardano-api as well?

@newhoggy
Copy link
Contributor Author

newhoggy commented Jul 1, 2025

The reason I want this is there are unexplained hangs and if watchdogProp can catch it I can log which test it is and there was one hang where this test was the last test that ran.

If watchdogProp doesn't wrap a test and I run a test and it hangs, I can't tell if it's one of the unwrapped tests.

The PR is not meant to be permanent. When the root cause of the hangs are determined, this can be rolled back and any changes upstreamed.

@newhoggy newhoggy requested review from carbolymer and Jimbo4350 July 1, 2025 10:47
@newhoggy newhoggy dismissed stale reviews from carbolymer and Jimbo4350 July 1, 2025 10:52

Replied

@newhoggy newhoggy force-pushed the newhoggy/local-error-golden-test-functions branch from 987346b to efdd3f1 Compare July 1, 2025 10:58
@newhoggy newhoggy force-pushed the newhoggy/local-error-golden-test-functions branch from efdd3f1 to 0d1f9c8 Compare July 1, 2025 10:59
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.

4 participants