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

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

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

Conversation

laurentsenta
Copy link
Contributor

@laurentsenta laurentsenta commented Feb 16, 2023

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:

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
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.

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.

test/sharness/t0046-id-hash.sh Outdated Show resolved Hide resolved
test/sharness/t0046-id-hash.sh Outdated Show resolved Hide resolved
@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.

grep -v "/ip4/1.2.3.4/tcp/1234" actual &&
grep -v "//ip4/10.20.30.40/tcp/4321" actual
test_should_not_contain "/ip4/1.2.3.4/tcp/1234" actual &&
test_should_not_contain "//ip4/10.20.30.40/tcp/4321" actual
Copy link
Contributor Author

@laurentsenta laurentsenta Aug 16, 2023

Choose a reason for hiding this comment

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

I kept it but that // looks like a typo

Copy link
Member

@lidel lidel Aug 17, 2023

Choose a reason for hiding this comment

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

Nice catch! Yes, it looks like copy&paste bug of sorts 🙈
Anyway, the test was broken because after #8177 we should test both addresses added via Announce and AppendAnnounce (so we need two specimens):

-noAnnounceCfg='["/ip4/1.2.3.4/tcp/1234"]'
+noAnnounceCfg='["/ip4/1.2.3.4/tcp/1234", "/ip4/10.20.30.40/tcp/4321"]'

Local test passed because other code filtered out non-LAN addr, but second one would fail, if we had no typo 🤯

I've pushed fix in 038e56d and should be green now 🤞

@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?

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.

2 participants