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

StaleIn can be function. #105

Merged
merged 1 commit into from Nov 19, 2014
Merged

Conversation

@jardakotesovec
Copy link
Contributor

jardakotesovec commented Oct 15, 2014

Replaces #94 #96 . Let me know if there is something to tweak. I did not want to get too crazy with tests, so let me know if you also find this as sufficient.

Thanks

@jardakotesovec jardakotesovec force-pushed the jardakotesovec:dynamic-staleIn branch from 9ea38d5 to e1e469f Oct 16, 2014
@jardakotesovec

This comment has been minimized.

Copy link
Contributor Author

jardakotesovec commented Oct 29, 2014

It would be great if this pull request would make it to hapi 8.0. Let me know if there is something I should change/improve.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 5, 2014

Need to fix the test for and coverage.

@hueniverse hueniverse added the feature label Nov 5, 2014
@hueniverse hueniverse self-assigned this Nov 5, 2014
@jardakotesovec

This comment has been minimized.

Copy link
Contributor Author

jardakotesovec commented Nov 5, 2014

If you run test on catbox Master it will fail occasionally. Likely because of tight timing.. I thought that I identified it in #106, but I just test this adjustment again and it still fails occasionally, so I probably just had lucky row..

Generally relying on 1ms difference in timing seems to be unreliable. I will try to identify which test(s) is causing this.

Is there actually trick how can I see which tests are going through particular line? That would help me to narrow the search.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 5, 2014

The problem is not timing but problem with the test under the new code module. Look at the specific error.

@jardakotesovec jardakotesovec force-pushed the jardakotesovec:dynamic-staleIn branch from e1e469f to 44c59cf Nov 5, 2014
@jardakotesovec

This comment has been minimized.

Copy link
Contributor Author

jardakotesovec commented Nov 5, 2014

Right, this is updated.

With regards to timing reliability. Tests on Master are reliable once I switch off everything (including spotify :) ), with higher cpu load timing gets crazy, which is fair enough. I understand that its about having balance between reliability and tests duration.

This pull request tend to fails occasionally (about 30% failure rate) with

101 tests complete
Test duration: 563 ms
No global variable leaks detected
Coverage: 99.70% (1/335)
lib/policy.js missing coverage on line(s): 120
Code coverage below threshold: 99.70 < 100

Which is when this if is not true:

        if (cached.ttl > 0) {

So I spend half day trying to understand the cause. I thought that typeof is slower or something, but it appeared that even if I just duplicate test (without changing it) on Master, it can cause the failures (I mean this particular failure above). It might be garbage collector that kicks in different time or something like that. I really don't know.

Anyway #106 updates timing for this particular failure, so it works properly. I am basically just trying justify here, that this occasional failure seems to be caused by adding tests, not by tests or code itself. I hope I am not wrong..

@hueniverse hueniverse added this to the 4.2.0 milestone Nov 19, 2014
hueniverse added a commit that referenced this pull request Nov 19, 2014
StaleIn can be function.
@hueniverse hueniverse merged commit 22064a6 into hapijs:master Nov 19, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.