Skip to content

test: fix "invert" commands in sharness tests #9652

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jun 9, 2025
Merged

test: fix "invert" commands in sharness tests #9652

merged 9 commits into from
Jun 9, 2025

Conversation

laurentsenta
Copy link
Contributor

@laurentsenta laurentsenta commented Feb 16, 2023 •
edited by gammazero
Loading

Many tests assume that grep --invert-match "X" FILE means

Fail when FILE contains "X"

But the command actually means:

Fail when FILE contains only "X"

Which is wrong in most cases.

  • We want to use test_should_not_contain instead.
  • We probably want to forbid the use of grep -v, it's confusing maintainers.

Example:

kubo/test/sharness/t0140-swarm.sh

Lines 95 to 109 in 3bcc89b

noAnnounceCfg='["/ip4/1.2.3.4/tcp/1234"]'
test_expect_success "test_config_set succeeds" "
ipfs config --json Addresses.NoAnnounce '$noAnnounceCfg'
"
test_launch_ipfs_daemon
test_expect_success "Addresses.NoAnnounce affects addresses from Announce and AppendAnnounce" '
ipfs swarm addrs local >actual &&
grep -v "/ip4/1.2.3.4/tcp/1234" actual &&
grep -v "/ip4/10.20.30.40/tcp/4321" actual &&
ipfs id -f"<addrs>" | xargs -n1 echo >actual &&
grep -v "/ip4/1.2.3.4/tcp/1234" actual &&
grep -v "//ip4/10.20.30.40/tcp/4321" actual
'

Test's Intent:

  1. add the multiaddr "/ip4/1.2.3.4/tcp/1234" to the list NoAnnounce,
  2. verify that the addr is not announced anymore.

But the command grep -v "/ip4/1.2.3.4/tcp/1234" actual succeeds even if the file contains /ip4/1.2.3.4/tcp/1234:

echo "/ip4/1.2.3.4/tcp/1234\nanything-else" > actual
grep -v "/ip4/1.2.3.4/tcp/1234" actual
# outputs "anything-else", exit code = 0

Which means that the assertion is a no-op.

suspects matching ack -l 'grep.*-v':

  • cmd/ipfs/Rules.mk
  • test/sharness/t0046-id-hash.sh
  • test/sharness/t0119-prometheus.sh
  • test/sharness/t0250-files-api.sh
  • test/sharness/t0060-daemon.sh
  • test/sharness/t0080-repo.sh
  • test/sharness/t0230-channel-streaming-http-content-type.sh
  • test/sharness/t0140-swarm.sh
  • test/sharness/t0141-addfilter.sh
  • test/sharness_test_coverage_helper.sh
  • bin/mkreleaselog
  • coverage/Rules.mk

Relates to #9651

@laurentsenta laurentsenta requested a review from lidel as a code owner February 16, 2023 11:16
@laurentsenta
Copy link
Contributor Author

@guseggert From your work on sharness tests, could you advise which tests we should investigate more? And which we'll drop soon?

I'm quite confident with the fix on t0118 because it reveals an error, so I know the new test is correct,
but for tests like t0046, the fix doesn't expose an error which hurts my red-green reflex.

lidel
lidel requested changes Aug 14, 2023
View reviewed changes
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@laurentsenta is this still something you want to do? If so, I think below miss &&, and that is why error produces false-positive.

@laurentsenta
Copy link
Contributor Author

I'll update this PR.

@lidel I think we still want to finish these, and maybe forbid the use of the --invert-match/-v it's confusing maintainers.

The tests here used grep -v "X" to check that X is NOT in the result,
but actually, the command succeeds as long as there are other lines.

echo "A\nB\nERROR" | grep -v "ERROR" 
echo $? # success because A anb B matches the regexp.

@laurentsenta laurentsenta requested a review from a team as a code owner August 16, 2023 13:32
laurentsenta
laurentsenta commented Aug 16, 2023
View reviewed changes
@lidel
Copy link
Member

lidel commented Aug 17, 2023

@laurentsenta yes, banning use of --invert-match/-v sgtm. We have somehow related pre-check in ./t0018-indent.sh, could you add something similar after we get rid of them, so it is not re-introduced?

@gammazero
Copy link
Contributor

gammazero commented Jun 3, 2025 •
edited
Loading

@lidel

could you add something similar

This is difficult because grep -v is used in a number of places to generate filtered output. This seems like something that requires the vigilance of reviewers.

gammazero
gammazero approved these changes Jun 3, 2025
View reviewed changes
Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

I verified, or added fixes for, the rest of the files using grep -v.

@gammazero gammazero requested a review from lidel June 3, 2025 21:59
lidel
lidel approved these changes Jun 9, 2025
View reviewed changes
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thanks!

@lidel lidel merged commit df2d1c7 into ipfs:master Jun 9, 2025
16 checks passed
@github-project-automation github-project-automation bot moved this from 🤝 Todo to 🥳 Done in InterPlanetary Developer Experience Jun 9, 2025
@BrewTestBot BrewTestBot mentioned this pull request Jul 14, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidel lidel lidel approved these changes

@gammazero gammazero gammazero approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@laurentsenta @lidel @gammazero