Skip to content

reftest: add test for variables resolutions in filter, for all fields #5643

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

Merged
merged 1 commit into from
Apr 9, 2025

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Aug 24, 2023

No description provided.

@rjbou rjbou force-pushed the test-resolve-variables-in-filters branch from 2e16743 to 0f61464 Compare August 24, 2023 15:59
@rjbou rjbou force-pushed the test-resolve-variables-in-filters branch from c191f42 to d53f6f9 Compare September 20, 2023 13:55
@rjbou rjbou force-pushed the test-resolve-variables-in-filters branch from d53f6f9 to 25837cf Compare December 12, 2023 10:13
@rjbou rjbou force-pushed the test-resolve-variables-in-filters branch from 25837cf to 930e28e Compare April 9, 2024 16:52
@rjbou rjbou requested review from kit-ty-kate and dra27 May 3, 2024 08:49
@kit-ty-kate kit-ty-kate removed their request for review May 9, 2024 13:24
Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

I haven't gone through the test properly, but unlike #5642, I think this one is a good candidate for a reftest. I half wondered with the tests which produce messages if it would be worth piping the whole thing through an appropriate grep just to keep the output smaller?

@rjbou rjbou added this to the 2.3 milestone Jun 27, 2024
@kit-ty-kate kit-ty-kate force-pushed the test-resolve-variables-in-filters branch from 97f6c16 to d2b04b6 Compare July 9, 2024 18:08
@rjbou rjbou marked this pull request as draft July 10, 2024 13:55
@rjbou rjbou force-pushed the test-resolve-variables-in-filters branch from d2b04b6 to a9c2b0a Compare August 29, 2024 18:11
@rjbou rjbou marked this pull request as ready for review August 29, 2024 18:11
@rjbou rjbou requested a review from kit-ty-kate August 29, 2024 18:11
Comment on lines 781 to 778
### <pkg:patches.1:some-content>
blabla
pioupiou
bloblob
### <pkg:patches.1:to-apply.patch>
--- a/some-content 2020-12-02 14:22:55.364620832 +0100
++ b/some-content 2020-12-02 14:23:05.668686881 +0100
@@ -1,3 +1,3 @@
blabla
-pioupiou
+ploplop
bloblob
### <pkg:patches.1:to-not-apply.patch>
--- a/some-content 2020-12-02 14:22:55.364620832 +0100
++ b/some-content 2020-12-02 14:23:05.668686881 +0100
@@ -2,3 +2,3 @@
ploplop
-bloblob
+noooooo
Copy link
Member

Choose a reason for hiding this comment

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

these files don't have to be redefined

Comment on lines +776 to +757
### :::::::::::::::::::::::::::
### :: Self package variable ::
### :::::::::::::::::::::::::::
Copy link
Member

Choose a reason for hiding this comment

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

for this section i think it would be also be useful to show the behaviour of opam when the package has already been installed. Like, for each fields, show the behaviour as is done currently, then force-install the package and try to install it again and show if there is a difference of behaviour

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated with these tests in a separate commit

@rjbou rjbou force-pushed the test-resolve-variables-in-filters branch from a9c2b0a to 4972b20 Compare October 14, 2024 17:38
Copy link
Collaborator Author

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

On file definition redundancy, the main idea is to have all information about the given test in its section. But there is 2 different cases:

  • patches that never changes, are always redefined, and their content it not so much important to understand the given test
  • opam files that are the same for a subset of the tests, they are more important to understand why it fails or not (how is used the variable).
    I think that patches can be grouped at the beginning of the test, but opam files add more readability if they are duplicated in their own section.

Comment on lines +776 to +757
### :::::::::::::::::::::::::::
### :: Self package variable ::
### :::::::::::::::::::::::::::
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated with these tests in a separate commit

@kit-ty-kate
Copy link
Member

i think this PR is the perfect way to start implementing the new testsuite schema we discussed about. We can use it to see how it works on a small scale, avoid duplication, and start the new testsuite work little by little

Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

[ "true" { ?smtg } ]
[ "false" { !?smtg } ]
]
### opam install runtest --with-test -vv | sed-cmd true | unordered
Copy link
Member

Choose a reason for hiding this comment

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

Are all the unordered necessary?

Suggested change
### opam install runtest --with-test -vv | sed-cmd true | unordered
### opam install runtest --with-test -vv | sed-cmd true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, -v displays the Processing [2/3...] and it is unreliable where the output is done (sometimes before the header...)

@rjbou
Copy link
Collaborator Author

rjbou commented Apr 9, 2025

rebased/fixed/updated

@rjbou rjbou force-pushed the test-resolve-variables-in-filters branch from 4972b20 to 6c626ef Compare April 9, 2025 15:20
@kit-ty-kate kit-ty-kate force-pushed the test-resolve-variables-in-filters branch from 6c626ef to 01e0659 Compare April 9, 2025 19:45
@kit-ty-kate
Copy link
Member

Thanks!

@kit-ty-kate kit-ty-kate merged commit fdfd129 into ocaml:master Apr 9, 2025
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants