Skip to content

tests: Print dots by default in functional tests #14569

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
maflcko merged 1 commit into from
Nov 1, 2018
Merged

tests: Print dots by default in functional tests #14569

maflcko merged 1 commit into from
Nov 1, 2018

Conversation

@ken2812221
Copy link
Contributor

@ken2812221 ken2812221 commented Oct 25, 2018

In cron job (https://travis-ci.org/bitcoin/bitcoin/builds/445823485), the functional tests would fail due to silent for 10 mins.

After applying this patch, we con't see any extra characters printed on screen but also avoid timeout (https://travis-ci.org/ken2812221/bitcoin/builds/445981698)

@fanquake fanquake added the Tests label Oct 25, 2018
@laanwj
Copy link
Member

laanwj commented Oct 25, 2018

For this reason the test runner prints . every so few seconds, but somehow—that's no longer enough?

@maflcko
Copy link
Member

maflcko commented Oct 25, 2018

This is probably due to if self.logging_level == logging.DEBUG: from #14504. Imo that condition can be removed.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 27, 2018 •
edited
Loading

No more conflicts as of last run.

@ken2812221 ken2812221 changed the title travis: Print characters per 9 min to avoid timeout travis: Print empty characters per 9 min to avoid timeout Oct 27, 2018
@DrahtBot DrahtBot mentioned this pull request Oct 27, 2018
Merged
@ken2812221
Copy link
Contributor Author

ken2812221 commented Oct 29, 2018

@MarcoFalke Is this what you mean or just remove the if condition to always print .?

@maflcko
Copy link
Member

maflcko commented Oct 29, 2018

Previously there was no condition, and it would always print: https://github.com/bitcoin/bitcoin/pull/14504/files?w=1#diff-a5b9b84e3a3387476629e74ddb227a7eR502

Not sure why that got added.

@Gnappuraz
Copy link
Contributor

Gnappuraz commented Oct 29, 2018 •
edited
Loading

@ken2812221 don't you have to kill the waiting at some point since you put it on background?
Also I think that travis has a 10m timeout, so printing every 9m should cut it.
I've done something like this to avoid it while running the tests...

if [ "$RUN_TESTS" = "true" ]; then
  while sleep 9m; do echo "=====[ $SECONDS seconds still running ]====="; done &
  make $MAKEJOBS check VERBOSE=1
  kill %1
fi

@isghe
Copy link
Contributor

isghe commented Oct 29, 2018 •
edited
Loading

Previously there was no condition, and it would always print: https://github.com/bitcoin/bitcoin/pull/14504/files?w=1#diff-a5b9b84e3a3387476629e74ddb227a7eR502

Not sure why that got added.

To be coherent with --quiet option "only print results summary and failure logs".

@ken2812221 Maybe we can add the --dots option to show the dots. So the travis command could be
DOCKER_EXEC test/functional/test_runner.py --combinedlogslen=4000 --coverage --quiet --failfast --dots ${extended};

or the --travis option.

maflcko
maflcko reviewed Oct 29, 2018
View reviewed changes
test/functional/test_runner.py Outdated
Copy link
Member

@maflcko maflcko Oct 29, 2018

Choose a reason for hiding this comment

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

I think it was nice to have some indicator that something was running. I'd prefer to keep it.

Copy link
Contributor

@isghe isghe Oct 30, 2018

Choose a reason for hiding this comment

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

I agree: running functional tests, without any progress indicator, it's not a good incentive to run functional tests.

@ken2812221 ken2812221 changed the title travis: Print empty characters per 9 min to avoid timeout tests: Print dots to avoid timeout when --dots given Oct 30, 2018
Gnappuraz
Gnappuraz approved these changes Oct 30, 2018
View reviewed changes
Copy link
Contributor

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

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

utACK 2e4435c

maflcko
maflcko reviewed Oct 30, 2018
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Sorry for nitpicking this trivial change to death. Feel free to ignore my feedback.

test/functional/test_runner.py Outdated
Copy link
Member

@maflcko maflcko Oct 30, 2018

Choose a reason for hiding this comment

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

self.logging_level is now unused. I'd slightly prefer to remove it.

test/functional/test_runner.py Outdated
Copy link
Member

@maflcko maflcko Oct 30, 2018

Choose a reason for hiding this comment

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

Is it really necessary to add an option to enable a feature that should be on by default? I think the average user would prefer to see dots without having to enable them by typing --dots every time.

test/functional/test_runner.py Outdated
Copy link
Member

@maflcko maflcko Oct 30, 2018

Choose a reason for hiding this comment

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

Suggested change
parser.add_argument('--quiet', '-q', action='store_true', help='only print results summary and failure logs')
parser.add_argument('--quiet', '-q', action='store_true', help='only print dots, results summary and failure logs')

Copy link
Member

@maflcko maflcko Oct 30, 2018

Choose a reason for hiding this comment

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

Could just add dots here and remove the other option?

@ken2812221 ken2812221 changed the title tests: Print dots to avoid timeout when --dots given tests: Print dots by default in functional tests Oct 31, 2018
@ken2812221
Copy link
Contributor Author

ken2812221 commented Oct 31, 2018

@MarcoFalke Now it print dots by default.

@maflcko
Copy link
Member

maflcko commented Oct 31, 2018

utACK 4bd125f

@fanquake
Copy link
Member

fanquake commented Oct 31, 2018

utACK 4bd125f

@jnewbery
Copy link
Contributor

jnewbery commented Nov 1, 2018

Seems reasonable. The whole point of quiet mode was to not fill the screen with output. Now that the dots erase themselves after each test, that's no longer an issues.

Tested ACK 4bd125f

@maflcko maflcko merged commit 4bd125f into bitcoin:master Nov 1, 2018
maflcko pushed a commit that referenced this pull request Nov 1, 2018
Merge #14569: tests: Print dots by default in functional tests
6a095bc 
4bd125f tests: Print dots by default (Chun Kuan Lee)

Pull request description:

  In cron job (https://travis-ci.org/bitcoin/bitcoin/builds/445823485), the functional tests would fail due to silent for 10 mins.

  After applying this patch, we con't see any extra characters printed on screen but also avoid timeout (https://travis-ci.org/ken2812221/bitcoin/builds/445981698)

Tree-SHA512: c0412e171a451b27f9734311c7f063ad3fd7142087ed1e3786b4f303acaebc043f970523d6c2d4ef57ec5857040e2b6f7fd6345304353e7805d76044d317344d
@ken2812221 ken2812221 deleted the travis-avoid-timeout branch November 1, 2018 15:06
@Warrows Warrows mentioned this pull request Jul 7, 2019
Merged
Fuzzbawls added a commit to PIVX-Project/PIVX that referenced this pull request Jul 11, 2019
@Fuzzbawls
Merge #943: [Travis] Show functional tests progress
918852c 
5fbf26c [Travis] Give more time to tests (warrows)
1aa76d0 travis: Use absolute paths for cache dirs (MarcoFalke)
1fa0bf3 travis: Fix caching issues (MarcoFalke)
e4945a2 [Travis] Log more info (warrows)
ae0b4d0 tests: Print remaining jobs in test_runner.py (Steven Roose)
a57939f tests: Print dots by default (Chun Kuan Lee)
cde9d13 show the progress of functional test (Isidoro Ghezzi)

Pull request description:

  Backport of bitcoin#14504, bitcoin#14569 and bitcoin#15466.
  This should help to see which test takes too long or blocks travis when it hangs because of the 50 minutes limit.

ACKs for commit 5fbf26:
  Fuzzbawls:
    ACK 5fbf26c

Tree-SHA512: 0c2bebd8f160e2c6358a598f3cf95c3451068af1b5e269d79366fe890f9609c140555cb182f0f4563dac35a0a433556eaf06e4a45865fd55bdd357130acc1d2c
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jun 28, 2021
@Munkybooty
Merge bitcoin#14569: tests: Print dots by default in functional tests
5d38c82 
4bd125f tests: Print dots by default (Chun Kuan Lee)

Pull request description:

  In cron job (https://travis-ci.org/bitcoin/bitcoin/builds/445823485), the functional tests would fail due to silent for 10 mins.

  After applying this patch, we con't see any extra characters printed on screen but also avoid timeout (https://travis-ci.org/ken2812221/bitcoin/builds/445981698)

Tree-SHA512: c0412e171a451b27f9734311c7f063ad3fd7142087ed1e3786b4f303acaebc043f970523d6c2d4ef57ec5857040e2b6f7fd6345304353e7805d76044d317344d

# Conflicts:
#	test/functional/test_runner.py
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jun 29, 2021
@Munkybooty
Merge bitcoin#14569: tests: Print dots by default in functional tests
a5f0708 
4bd125f tests: Print dots by default (Chun Kuan Lee)

Pull request description:

  In cron job (https://travis-ci.org/bitcoin/bitcoin/builds/445823485), the functional tests would fail due to silent for 10 mins.

  After applying this patch, we con't see any extra characters printed on screen but also avoid timeout (https://travis-ci.org/ken2812221/bitcoin/builds/445981698)

Tree-SHA512: c0412e171a451b27f9734311c7f063ad3fd7142087ed1e3786b4f303acaebc043f970523d6c2d4ef57ec5857040e2b6f7fd6345304353e7805d76044d317344d

# Conflicts:
#	test/functional/test_runner.py
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@isghe isghe isghe left review comments

@maflcko maflcko maflcko left review comments

@Gnappuraz Gnappuraz Gnappuraz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ken2812221 @laanwj @maflcko @DrahtBot @Gnappuraz @isghe @fanquake @jnewbery