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

Reftests: factorise some repo hashes #5380

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Dec 6, 2022

No description provided.

@kit-ty-kate
Copy link
Member

Have you checked that those two tests still have the same behaviours if this patch was applied before their respective fixes?

@rjbou
Copy link
Collaborator Author

rjbou commented Dec 7, 2022

conflict-4373 has no core modifications, and list just ec6f65e, and changing repo hash doesn't change the output.

@rjbou rjbou added the PR: WIP Not for merge at this stage label Feb 15, 2023
@rjbou
Copy link
Collaborator Author

rjbou commented Feb 15, 2023

#5414 used in another test a removed hash in this PR. Need to be reviewed

@kit-ty-kate
Copy link
Member

kit-ty-kate commented Feb 15, 2023

I’m not sure it’s worth potentially breaking carefully crafted tests just to save 3 secondes (i tried) in total test time

@rjbou
Copy link
Collaborator Author

rjbou commented Feb 15, 2023

What is broken ?

@kit-ty-kate
Copy link
Member

I don’t know, I saying potentially. When writing tests there are many factors that appears when choosing a repository hash, many of which might not appear in the results before or after the commit that added the test and its fix.

To me it’s not worth the hassle.

@rjbou
Copy link
Collaborator Author

rjbou commented Feb 15, 2023

In the current state of the PR, I checked that it is the same before and after fix, when there is an after

@kit-ty-kate
Copy link
Member

many of which might not appear in the results before or after the commit

as i said, many invariants do not appear in the results before or after

@rjbou
Copy link
Collaborator Author

rjbou commented Feb 16, 2023

oups, i misread. Then what are the other things that you check when choosing an hash, except that it permits you to have the "good" test output ?

@kit-ty-kate kit-ty-kate removed their request for review May 9, 2024 13:24
@rjbou rjbou marked this pull request as draft July 10, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: WIP Not for merge at this stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants