Reftest: add more exhaustive pin command test #6135

rjbou · 2024-07-31T17:53:19Z

This is separated in 2 commits for readability/review:

first that adds a test for all pin subcommands and options;
second that update previous test (that were only special cases and new subcommands) to make them independent, each one is run in a fresh switch.

I don't know if I added a test for each case (exhaustiveness).

Backported for 2.3 in #6229

~~Queued on #6219~~ Unqueued it

tests/reftests/pin.test

rjbou · 2024-07-31T17:55:23Z

tests/reftests/pin.test

-### : Test opam pin remove <pkg>.<version>
-### opam pin remove --no-action bar.dev
-Ok, bar is no longer pinned to file://${BASEDIR}/bar (version dev)
+### :XI:c: Test opam pin remove <pkg>.<version>


Should this test & remove --all one be in its proper section above ?

i think it's fine here

rjbou · 2024-07-31T17:56:58Z

tests/reftests/pin.test

-### opam pin add --no-action bar.dev ./bar
+### opam pin remove --no-action nip2.dev
+[NOTE] nip2 is not pinned.
+### :XI:d: Test opam pin remove --all


I kept this test here, "as is": be the most similar to previous one with several kind of pins, etc. I don't know if it is intended or not?

i'm not sure to understand what you mean here

Is it intended to have different kind of pins to remove ? or it was the case because the current switch had already several kind of pins?

rjbou · 2024-07-31T17:57:29Z

tests/reftests/pin.test

+### opam pin add ./nip-empty2
+Package nip-empty2 does not exist, create as a NEW package? [y/n] y
+nip-empty2 is now pinned to file://${BASEDIR}/nip-empty2 (version dev)
+
+The following actions will be performed:
+=== install 1 package
+  - install nip-empty2 dev (pinned)
+
+<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+-> retrieved nip-empty2.dev  (file://${BASEDIR}/nip-empty2)
+-> installed nip-empty2.dev
+Done.


Is it a bug?

the fact that linting is not done if the name of the package is not specified? yeah probably although it doesn't seem critical

rjbou · 2024-07-31T17:58:01Z

tests/reftests/pin.test

+### opam install ./nip-fo
+[NOTE] Ignoring uncommitted changes in ${BASEDIR}/nip-fo (`--working-dir' not specified or specified with no argument).
+[nip-fo.dev2] synchronised (git+file://${BASEDIR}/nip-fo#master)
+The following actions will be performed:
+=== recompile 1 package
+  - recompile nip-fo dev2 (pinned)
+
+<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+-> removed   nip-fo.dev2
+-> installed nip-fo.dev2
+Done.
+### opam show nip-fo --field=version
+dev2


Is it a bug?

yes it is a known bug, i've seen it several times but i can't find the issue that i used to redirect people to when they got that issue

#5567 (comment) maybe?

tests/reftests/pin.test

kit-ty-kate

LGTM aside from those two things and the first commit message

tests/reftests/pin.test

master_changes.md

tests/reftests/pin.test

rjbou · 2024-10-02T12:52:22Z

tests/reftests/pin.test

+Done.
+### opam pin edit nip-path
+[WARNING] The opam file didn't pass validation:
+           warning 59: url doesn't contain a checksum


This will be fixed with #6218

master_changes.md

kit-ty-kate · 2024-10-02T14:17:33Z

tests/reftests/pin.test

+### opam switch create removal --empty
+### opam pin ./nip --no-action
+nip is now pinned to file://${BASEDIR}/nip (version ved)
+### opam pin remove --no-action nip.dev


This command is duplicated with the nip.wrong-version test below

Suggested change

### opam pin remove --no-action nip.dev

In fact, it is

### opam pin remove --no-action nip.ved

The logic

test with a good version of pinned packages

test with a wrong version of pinned packages

test with a not pinned package

it's not. It was:

remove good version of pkg1

remove wrong version of pkg2

Thus we should simplify into:

remove wrong version of pkg1

remove good version of pkg1

In the original test i agree it was : remove good version then remove wrong version. I've updated the test to add also removal of unpinned one.

"The logic" i was talking about is on the wanted final test (my modif), not the original.

i don't think the extra test makes sense and i don't think we should add new tests here. The commit changing this part is here just to add separation not adding extra tests. It's already complicated to review changes in the reftests as it is.

In the final form (not yet pushed, i'm still debugging cygwin), i've separated the addition of unpinned package test in first commit like that the second commit only contains the switch change.

kit-ty-kate · 2024-10-02T14:20:52Z

tests/reftests/pin.test

 ### opam pin remove --no-action nip.wrong-version
 [ERROR] nip is pinned but not to version wrong-version. Skipping.
 # Return code 2 #
+### opam pin remove --no-action nip2.dev


The version of nip is ved not dev. The test is different from originally intended

Suggested change

### opam pin remove --no-action nip2.dev

### opam pin remove --no-action nip.ved

kit-ty-kate · 2024-10-02T14:22:33Z

tests/reftests/pin.test

+### opam pin ./nip --no-action
+[NOTE] Package nip is already pinned to file://${BASEDIR}/nip (version ved).
+nip is now pinned to file://${BASEDIR}/nip (version ved)


this is not needed since the pinned package hasn't been removed

Suggested change

### opam pin ./nip --no-action

[NOTE] Package nip is already pinned to file://${BASEDIR}/nip (version ved).

nip is now pinned to file://${BASEDIR}/nip (version ved)

kit-ty-kate

I'm personally not a huge fan of editing the testsuite. I would've preferred to have a new file or to rename the old one if you prefer, but changing it is hard to review and can potentially break test-cases (the opam pin remove <pkg>.<version> section was a good example). In my opinion our testsuite should be append-only whenever possible in the future.

In any case i won't block this. It looks fine as far as i could review.
Thanks!

rjbou · 2024-10-07T15:15:24Z

Updated with the news test in their own file,like that the legacy remains untouched. I just updated the comments and added a test case for remove <pkg.version>. The content of the new file is the same than previous versions of this PR.

kit-ty-kate · 2024-10-07T15:16:28Z

tests/reftests/pin.test

+### # shell on Cygwin ads a trialing `\r` in the middle of the string
+### sh -c 'opam pin add $(opam pin scan ./lot_of_pkgs --normalise | grep path | sed "s/\\r//g") ' | unordered


What about:

Suggested change

### # shell on Cygwin ads a trialing `\r` in the middle of the string

### sh -c 'opam pin add $(opam pin scan ./lot_of_pkgs --normalise | grep path | sed "s/\\r//g") ' | unordered

### sh -c 'opam pin scan ./lot_of_pkgs --normalise | xargs opam pin add'

it doesn't work neither. If there is at least 2 packages, the \r issue is present

interesting

kit-ty-kate · 2024-10-07T16:50:58Z

Thanks a lot!

rjbou added the AREA:TEST label Jul 31, 2024

rjbou added this to the 2.3.0~alpha milestone Jul 31, 2024

rjbou requested review from kit-ty-kate and dra27 July 31, 2024 17:53

rjbou commented Jul 31, 2024

View reviewed changes

kit-ty-kate reviewed Jul 31, 2024

View reviewed changes

tests/reftests/pin.test Outdated Show resolved Hide resolved

tests/reftests/pin.test Outdated Show resolved Hide resolved

kit-ty-kate reviewed Jul 31, 2024

View reviewed changes

master_changes.md Outdated Show resolved Hide resolved

kit-ty-kate reviewed Aug 1, 2024

View reviewed changes

tests/reftests/pin.test Outdated Show resolved Hide resolved

kit-ty-kate modified the milestones: 2.3.0~alpha1, 2.4.0~alpha1 Sep 18, 2024

rjbou force-pushed the pin-test branch from 40c3037 to 3caec6b Compare October 1, 2024 10:11

rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Oct 1, 2024

rjbou modified the milestones: 2.4.0~alpha1, 2.3.0~alpha2 Oct 1, 2024

rjbou requested a review from kit-ty-kate October 1, 2024 10:11

kit-ty-kate modified the milestones: 2.3.0~alpha2, 2.4.0~alpha1 Oct 1, 2024

rjbou mentioned this pull request Oct 1, 2024

Pin: fix first local vcs fetch rsyncing all source #6221

Merged

rjbou marked this pull request as ready for review October 2, 2024 12:49

rjbou force-pushed the pin-test branch from 3caec6b to 3547f00 Compare October 2, 2024 12:51

rjbou removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Oct 2, 2024

rjbou commented Oct 2, 2024

View reviewed changes

rjbou force-pushed the pin-test branch from 3547f00 to f6f5cfc Compare October 2, 2024 12:54

kit-ty-kate reviewed Oct 2, 2024

View reviewed changes

master_changes.md Outdated Show resolved Hide resolved

rjbou force-pushed the pin-test branch from f6f5cfc to bb99b29 Compare October 2, 2024 12:56

kit-ty-kate requested changes Oct 2, 2024

View reviewed changes

rjbou force-pushed the pin-test branch from bb99b29 to 6ced7f4 Compare October 4, 2024 17:48

kit-ty-kate force-pushed the pin-test branch from 6ced7f4 to 3077874 Compare October 5, 2024 02:34

kit-ty-kate approved these changes Oct 5, 2024

View reviewed changes

kit-ty-kate added the STATE: READY TO MERGE label Oct 5, 2024

rjbou added 2 commits October 7, 2024 17:10

reftest: move pin test to pin-legacy

8e3b634

reftest: more exhaustive test for pin command

ab8c465

rjbou force-pushed the pin-test branch from 3077874 to ab8c465 Compare October 7, 2024 15:13

kit-ty-kate reviewed Oct 7, 2024

View reviewed changes

kit-ty-kate approved these changes Oct 7, 2024

View reviewed changes

kit-ty-kate merged commit c048c05 into ocaml:master Oct 7, 2024
29 checks passed

rjbou mentioned this pull request Oct 7, 2024

[2.3] Reftest: add more exhaustive pin command test #6229

Merged

rjbou changed the title ~~Reftest: add pin command test~~ Reftest: add more exhaustive pin command test Oct 7, 2024

kit-ty-kate mentioned this pull request Oct 16, 2024

reftest: add admin filter test #6166

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reftest: add more exhaustive pin command test #6135

Reftest: add more exhaustive pin command test #6135

rjbou commented Jul 31, 2024 •

edited

Loading

rjbou Jul 31, 2024 •

edited

Loading

kit-ty-kate Jul 31, 2024

rjbou Jul 31, 2024

kit-ty-kate Jul 31, 2024

rjbou Oct 1, 2024

rjbou Jul 31, 2024

kit-ty-kate Jul 31, 2024

rjbou Jul 31, 2024

kit-ty-kate Jul 31, 2024

kit-ty-kate Jul 31, 2024

kit-ty-kate left a comment

rjbou Oct 2, 2024

kit-ty-kate Oct 2, 2024

rjbou Oct 2, 2024

kit-ty-kate Oct 2, 2024

rjbou Oct 2, 2024

kit-ty-kate Oct 2, 2024

rjbou Oct 2, 2024

kit-ty-kate Oct 2, 2024

rjbou Oct 2, 2024

kit-ty-kate Oct 2, 2024

rjbou Oct 2, 2024

kit-ty-kate left a comment

rjbou commented Oct 7, 2024

kit-ty-kate Oct 7, 2024

rjbou Oct 7, 2024 •

edited

Loading

kit-ty-kate Oct 7, 2024

kit-ty-kate commented Oct 7, 2024

	### opam pin remove --no-action nip2.dev
	### opam pin remove --no-action nip.ved

	### opam pin ./nip --no-action
	[NOTE] Package nip is already pinned to file://${BASEDIR}/nip (version ved).
	nip is now pinned to file://${BASEDIR}/nip (version ved)

		### # shell on Cygwin ads a trialing `\r` in the middle of the string
		### sh -c 'opam pin add $(opam pin scan ./lot_of_pkgs --normalise \| grep path \| sed "s/\\r//g") ' \| unordered

	### # shell on Cygwin ads a trialing `\r` in the middle of the string
	### sh -c 'opam pin add $(opam pin scan ./lot_of_pkgs --normalise \| grep path \| sed "s/\\r//g") ' \| unordered
	### sh -c 'opam pin scan ./lot_of_pkgs --normalise \| xargs opam pin add'

Reftest: add more exhaustive pin command test #6135

Reftest: add more exhaustive pin command test #6135

Conversation

rjbou commented Jul 31, 2024 • edited Loading

rjbou Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kit-ty-kate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kit-ty-kate left a comment

Choose a reason for hiding this comment

rjbou commented Oct 7, 2024

Choose a reason for hiding this comment

rjbou Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kit-ty-kate commented Oct 7, 2024

rjbou commented Jul 31, 2024 •

edited

Loading

rjbou Jul 31, 2024 •

edited

Loading

rjbou Oct 7, 2024 •

edited

Loading