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

Wip/mango 1945/fix failing tests #112

Open
wants to merge 1 commit into
base: feat/MANGO-1814/bind-address-handling
Choose a base branch
from

Conversation

kishorevenki
Copy link

@kishorevenki kishorevenki commented Feb 10, 2025

  • This fix addresses the issues mentioned in https://radixiot.atlassian.net/browse/MANGO-1945 .
  • Two tests from TrendLogObjectTest and one test from TrendLogObjectMultipleTest were failing.
  • The intent of these tests are to verify that data are collected at periodic interval as per Log-Interval property.
  • While TrendObjects are collecting the data for every minute, the test incorrectly change the log interval to one hour. Collection of records and hence the record-count cannot be accurate if the log interval property is changed while TrendLog objects are collecting the live data. Further such step is not the standard test requirement.
  • Tests are fixed by removing the steps of chage of Log-Interval property while collecting the data.

@kishorevenki kishorevenki changed the base branch from master to feat/MANGO-1814/bind-address-handling February 11, 2025 04:35
@kishorevenki kishorevenki marked this pull request as ready for review February 11, 2025 06:22
@terrypacker
Copy link

terrypacker commented Feb 11, 2025

I checked out the code and dug around a little. Instead of removing some of the coverage can we just move those 1hr test cases into separate tests? I get a little nervous when I see the failing code of a test just deleted. I think I understand why it is failing but since we are using a WarpClock there is probably a few ways to make this work without removing the code.

From what I can tell this is really the code you removed that you are concerned with:

        // Advance the clock to the new polling time.
        final int minutes = (62 - clock.get(ChronoField.MINUTE_OF_HOUR)) % 60;
        clock.plus(minutes, MINUTES, 0);

I'm thinking if you just set the time to a nice round number at the start of the test or account for why the above could be off by 1m2s ever.

However I would be ok with just having a separate tests for the 1hr polling interval.

@kishorevenki
Copy link
Author

Adding a separate test for one hour polling interval would be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants