Opened 19 months ago

Last modified 10 months ago

#21171 new enhancement

Write a test for NodeDetailsStatusUpdater

Reported by: iwakeh Owned by: metrics-team
Priority: Medium Milestone:
Component: Metrics/Onionoo Version:
Severity: Normal Keywords: metrics-help
Cc: iwakeh, kiki201 Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

concentrate on bugfix #20994 and try to keep the test concise
(it might be necessary to use reflection)

Child Tickets

Change History (8)

comment:1 Changed 14 months ago by iwakeh

Cc: iwakeh added

Adding myself to cc to make trac mail updates.

comment:2 Changed 14 months ago by kiki201

Cc: kiki201 added

comment:3 Changed 14 months ago by kiki201

Do I need to create mock objects for ReverseDomainNameResolver, LookupService, Descriptor and other classes?

Do I need to use reflection to test private methods and do I even need to test private methods?

These are questions that I had sent to the metrics mailing list. It was inappropriate to send them there so I'm asking them here instead.

comment:4 in reply to:  3 ; Changed 13 months ago by iwakeh

Replying to kiki201:

Do I need to create mock objects for ReverseDomainNameResolver, LookupService, Descriptor and other classes?

Only, if you need these to test the fix supplied in #20994.

Do I need to use reflection to test private methods and do I even need to test private methods?

Same here, if it provides a shortcut and turns out to be easier you can use reflection, no must.

As the description of this ticket is quite terse, I try to give some more details:

Ticket #20994#comment:5 explains the bug, its fix, and supplies the fixed code, but no test.
So, the aim is to device a test that fails on code before this commit (and because of the reason described in #20994#comment:5).
There are no requirements of having to use mock objects or reflection, but they are fine to use for making the test code simpler.
This might turn out tricky, just feel free to experiment.

comment:5 in reply to:  4 Changed 13 months ago by kiki201

Replying to iwakeh:

Replying to kiki201:

Do I need to create mock objects for ReverseDomainNameResolver, LookupService, Descriptor and other classes?

Only, if you need these to test the fix supplied in #20994.

Do I need to use reflection to test private methods and do I even need to test private methods?

Same here, if it provides a shortcut and turns out to be easier you can use reflection, no must.

As the description of this ticket is quite terse, I try to give some more details:

Ticket #20994#comment:5 explains the bug, its fix, and supplies the fixed code, but no test.
So, the aim is to device a test that fails on code before this commit (and because of the reason described in #20994#comment:5).
There are no requirements of having to use mock objects or reflection, but they are fine to use for making the test code simpler.
This might turn out tricky, just feel free to experiment.

I didn't understand completely what the test is supposed to do. I am supposed to write a test that fails on the code before the given commit. Does the test checks if the first_seen timestamp on bridges is not "1970-01-01 00:00:00" ? Or does the test have more to do with the lines that were commented out in #20994? Also, since this is on an old version of the code, I'm going to have to checkout in this branch. Is this correct?

Thank you

comment:6 Changed 13 months ago by iwakeh

Right, you can checkout the commit before the one given above or make the changes by hand; whatever you prefer. (there is only the master branch in the main repo.)

Regarding the bug in #20994: the error was to save nodes that never showed up in a consensus (cf. comment 3, there).

Just ask again, if anything is unclear.

comment:7 Changed 13 months ago by kiki201

So, the test should check if a consensus doesn't contain nodes that never showed up, so that the code before the commit fails. Is this right?

However, I'm not sure about how to go about testing this. The project contains a mock consensus object that implements RelayNetworkStatusConsensus. But, RelayNetworkStatusConsensus is only used in processDescriptor. So, I don't know how to take advantage of this class. I'm not sure how to simulate the whole process in order to test it.

comment:8 Changed 10 months ago by karsten

Summary: write a test for NodeDetailsStatusUpdaterWrite a test for NodeDetailsStatusUpdater

Capitalize summary.

Note: See TracTickets for help on using tickets.