This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add tests to characterise the current behaviour of R30 phone-home metrics #10315
Add tests to characterise the current behaviour of R30 phone-home metrics #10315
Changes from 6 commits
071037c
e3f6af7
2082de4
9a88346
b6a0093
9753b8b
cc6b61c
f60b30f
3caa8d6
d3f90a7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that R30 doesn't work out of the box?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default is 28d, so out of the box, it does seem like it's slightly broken, yes...
Out of the box, it can only count users who had activity in the last 28 days, where that activity occurred at least 30 days after registration.
The intention would be to count users who had activity in the last 30 days, where that activity occurred at least 30 days after registration, so it's not a MASSIVE difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just wary of testing a feature by first changing the default config? Can we update the test to work with the default, or do we need to update the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, the default should be changed. (However, I'm not sure how worthwhile that is since the idea is to add R30v2 and eventually remove R30 [I expect]...).
Updating the test to work with the default wouldn't really show what R30 actually means, it'd instead be R28 or something ;p.
I was hoping to basically just add a test to characterise the current behaviour and wasn't intending to change anything, though...
Suppose I could make a copy of the test that uses the default settings? What do you think is preferable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess its fair to merge as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, yes, I think we should have a test for the default, since this is for phone home stats.