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

convert more tests to use the sharness framework #103

Merged
merged 9 commits into from
Jan 29, 2024

Conversation

garlick
Copy link
Member

@garlick garlick commented Jan 28, 2024

Just moving some more tests into sharness.

Also: gitignore updated, and recently added --reconnect-retry delay adjusted to avoid excessive waiting.

Problem: gitignore files are out of date and do not include
TAP test products.

Update gitignores.
Problem: --retry-connect=N sleeps too long if the first connect
doesn't succeed, causing tests to take longer than necessary.

Instead of sleeping 1s bewteen connects, sleep 0.1s.

Update test.
@garlick
Copy link
Member Author

garlick commented Jan 28, 2024

I've got some more sharness conversion stuff pending so I'll just tack it on here.

Copy link
Member

@chu11 chu11 left a comment

Choose a reason for hiding this comment

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

LGTM, just the small things mentioned below (some may be for a follow on PR for more coverage)

Comment on lines +91 to +96
test_expect_success 'telemetry has expected size' '
test $(stat -c%s tel2.out) -eq 1926
'
Copy link
Member

Choose a reason for hiding this comment

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

i think this approach is fine, but maybe a comment that says generally what is expected of the output? Since to the uniniated, they got no idea what this test does and what "spew" does. Maybe something like

send> on
abcdefghfdfdfadsfdsafdsafdsfsda .... (X times)
recv> ...
on: ...
off: ...

@@ -0,0 +1,114 @@
#!/bin/sh

test_description='Test Powerman status query'
Copy link
Member

@chu11 chu11 Jan 29, 2024

Choose a reason for hiding this comment

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

not necessarily for this PR, but do you need to cover the "pm -q foo[1-3]" case too? Get all status and then filter it down? TBH, I didn't even know about the -Q option, I always do pm -q foo[1-3] :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah might as well!

I'm getting reacquainted with the project too. The tests before were super inscrutable and now they at least tell you kind of what they are doing and can be more easily grouped functionally.

Comment on lines +29 to +38
test_expect_success 'powerman -r t1 works' '
$powerman -h $testaddr -r t1 >reset1.out &&
echo "Command completed successfully" >reset1.exp &&
test_cmp reset1.exp reset1.out
'
test_expect_success 'powerman -r t[3-5] works' '
$powerman -h $testaddr -r t[3-5] >reset2.out &&
echo "Command completed successfully" >reset2.exp &&
test_cmp reset2.exp reset2.out
'
Copy link
Member

Choose a reason for hiding this comment

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

not ncessarily for this PR as you're just converting old tests, but should we be checking on/off before and after doing these?

Copy link
Member Author

Choose a reason for hiding this comment

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

No reset doesn't affect on/off. It's an optional hardware reset (like press the reset button).

Problem: tests/t06, tests/t07, and tests/t12 cover powerman status
query using the old test infrastructure.

Convert to sharness.

Remove the old test scripts and data.
Problem: tests/t08, t09, t10, t17, and t18 cover powerman telemetry
output using the old test infrastructure.

Convert to sharness.

Be lazy and just check the output size rather than check that the content
exactly matches like the old test.  I think this fulfills the goal of
the test.

Remove the old test scripts and data.
Problem: Problem: tests/t11 covers powerman on/off/cycle operations
using the old test infrastructure.

Convert to sharness.

Remove the old test scripts and data.
Problem: tests/t14 covers powerman temperature query
using the old test infrastructure.

Convert to sharness.

Remove the old test scripts and data.
Problem: tests/t13 covers powerman beacon control
using the old test infrastructure.

Convert to sharness.

Remove the old test scripts and data.
Problem: tests/t15 covers powerman reset control
using the old test infrastructure.

Convert to sharness.

Remove the old test scripts and data.
Problem: the list of tests in test/README is out of date.

Remove the tests that have been converted to sharness thus far.
@garlick
Copy link
Member Author

garlick commented Jan 29, 2024

Thanks! I pushed those fixes and will set MWP.

@mergify mergify bot merged commit d8db6bf into chaos:master Jan 29, 2024
8 checks passed
@garlick garlick deleted the sharness_conv2 branch January 29, 2024 01:12
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.

None yet

2 participants