Opened 4 months ago

Closed 3 months ago

#30455 closed enhancement (fixed)

Improve documentation for chutney warnings in "make test-network-all"

Reported by: nickm Owned by: teor
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.3.0.4-rc
Severity: Normal Keywords: chutney, easy, doc, tor-ci, asn-merge
Cc: teor Actual Points: 0.1
Parent ID: Points: 0.1
Reviewer: nickm Sponsor: Sponsor19-can

Description

It appears that in fb32c522320430f, we added a second call to test-network.sh inside our test-network-all loop. Now the code looks like this:

	for f in $$flavors; do \
		$(SHELL) $(top_srcdir)/test-driver --test-name $$f --log-file $(TEST_NETWORK_ALL_LOG_DIR)/$$f.log --trs-file $(TEST_NETWORK_ALL_LOG_DIR)/$$f.trs $(TEST_NETWORK_ALL_DRIVER_FLAGS) $(top_srcdir)/src/test/test-network.sh --flavor $$f $(TEST_NETWORK_FLAGS); \
		$(top_srcdir)/src/test/test-network.sh $(TEST_NETWORK_WARNING_FLAGS); \
	done; \

I might be wrong, but it looks to me like we're calling test-network.sh twice in each loop: once through test-driver, and once directly.

I'm not going to work on this till teor is back, though, since there are dragons here that I do not understand.

Child Tickets

Change History (11)

comment:1 Changed 4 months ago by nickm

Keywords: tor-ci added

comment:2 Changed 4 months ago by nickm

Keywords: 041-should added

comment:3 Changed 4 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:4 Changed 4 months ago by nickm

Findings so far:

  • This bug was new in 0.3.0.4.
  • We are indeed trying to run everything twice.
  • We are not actually running everything twice, since TEST_NETWORK_WARNING_FLAGS seems to be broken: it uses --only-warnings, which appears to prevent the test from running entirely.

Any other fixes here should probably come after we merge our other "use chutney for CI" tickets, since they are likely to touch this part of the makefile as well.

comment:5 in reply to:  4 Changed 4 months ago by teor

Keywords: doc added; 035-backport? 040-backport? removed
Points: 0.1
Summary: Does "make test-network-all" test every network twice?Improve documentation for chutney warnings in "make test-network-all"
Type: defectenhancement

Replying to nickm:

Findings so far:

  • This bug was new in 0.3.0.4.
  • We are indeed trying to run everything twice.
  • We are not actually running everything twice, since TEST_NETWORK_WARNING_FLAGS seems to be broken: it uses --only-warnings, which appears to prevent the test from running entirely.

This is not a bug.

The extra line logs chutney warnings to the terminal. Before this change, tor's warnings were only logged to a file. And if chutney succeeded, no-one would read that file. So we would miss any warnings that tor had logged.

For more details, see #21570 or the changes file in commit fb32c522320430f.

Any other fixes here should probably come after we merge our other "use chutney for CI" tickets, since they are likely to touch this part of the makefile as well.

We could change the variable name so it is move obvious: TEST_NETWORK_SHOW_LAST_WARNINGS_FLAGS
And we could add a comment above the makefile block.

comment:6 Changed 4 months ago by nickm

Owner: changed from nickm to teor
Status: acceptedassigned

Passing this over to teor, since you seem to understand it better than I did -- please pass it back if you don't want it. :)

comment:7 Changed 3 months ago by teor

Actual Points: 0.1
Sponsor: Sponsor19-can
Status: assignedneeds_review
Version: Tor: 0.3.0.4-rc

I think we discovered this issue during Sponsor 19 work.

See my pull request:

Test merges:

Any other fixes here should probably come after we merge our other "use chutney for CI" tickets, since they are likely to touch this part of the makefile as well.

We decided to use "make test-network-all" without any modifications, so there shouldn't be any conflicts.

comment:8 Changed 3 months ago by dgoulet

Reviewer: nickm

comment:9 Changed 3 months ago by nickm

Keywords: 041-should removed
Milestone: Tor: 0.4.1.x-finalTor: 0.4.2.x-final
Status: needs_reviewmerge_ready

LGTM; I suggest merging to master only.

comment:10 Changed 3 months ago by nickm

Keywords: asn-merge added

comment:11 Changed 3 months ago by asn

Resolution: fixed
Status: merge_readyclosed

Great stuff. Merged to master.

Note: See TracTickets for help on using tickets.