-
Notifications
You must be signed in to change notification settings - Fork 93
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
Platform from platform group basic unit test #3832
Platform from platform group basic unit test #3832
Conversation
- store platform name - add test for random platform selection
🤣 |
Do you mean move it out of the "flaky" category? (Not delete it outright). If so, yes - going by your description of the chance failure, it is now the least flaky functional test in the entire universe. [UPDATE: it's not entirely clear to me from your description if that applies to the functional test or the unit tests??] |
I mean delete |
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 have checked out your branch, read the code and run the tests. I am happy with the change and I think it makes sense to move these tests to a new file.
With regards to the flaky functional test, I think, if I have understood things correctly, that test can be deleted - a flakiness number of 100 is preferential to 9, and I think the unit tests now have this covered.
names = [] | ||
# this number keeps the test fast but give about 1.6x10^-30 chance of | ||
# a false negative. | ||
flakiness_n = 100 |
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.
Nice to be able to increase the flakiness number from 9 to 100.
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.
Given the speed of pytest it could go a lot higher without being a problem, but 10^-30 seems sufficient. It's probably less likely than computer running the test bursting into flames whilst running the test.
Co-authored-by: Melanie Hall <37735232+datamel@users.noreply.github.com>
Co-authored-by: Melanie Hall <37735232+datamel@users.noreply.github.com>
…platform::test_get_platform_from_group_basic
…lc into platforms.aliases.add_unit_test * 'platforms.aliases.add_unit_test' of github.com:wxtim/cylc: Update tests/unit/test_platforms_get_platform.py Update tests/unit/test_platforms_get_platform.py
flakiness = 2 / 2 ** flakiness_n | ||
for i in range(flakiness_n): | ||
names.append(get_platform('hebrew_letters')['name']) | ||
if 'aleph' in names and 'bet' in names: | ||
break |
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.
With this test written in Python there might be a nicer solution, the Python random library is only psudo-random. You can control the series of random numbers it produces by setting the random seed.
random.seed(1234)
assert [
get_platform('hebrew_letters')['name']
for _ in range(N)
] == [
# results should come out in the same order each time
]
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.
This will, I think work rather well. Thanks for the point!
I tried it and it works. I am amused that this testing approach takes about 4x the time the earlier rather crude version did.
Yes, I think we've gone to great lengths to prove that the random method does indeed work. |
* master: (36 commits) Cleaned up logging a little (cylc#3836) Platform from platform group basic unit test (cylc#3832) added reqd test_header tid platform tests scan: improve colour output spelling error fix scan: fix hier suites test: old host/batch system configs don't conflict with dummy-local mode Fix task message validation for multiline messages Fix spelling error. removed cylc7 data structure removed unused endpoints Commands migrated to GraphQL endpoint cli: rename spawn -> set-outputs Update changelog Remove test_unicode_rules Do not allow any colons in task outputs Fix command help. Correct typo. Upgraded new test. ...
Precursor to #3827
Partial fix of issues outstanding from #3793
store platform group name
Probably required for #3827
refactor platforms tests
The file
tests/unit/test_platforms.py
had become large. I moved tests of the functionget_platform
to a separate file:test_platforms_get_platform.py
add test for random platform selection
The test for random platform selection
tests/flakyfunctional/platform_aliases/00-simple.t
makes a trade off between run time and probability of giving a false negative. For some reason I failed to use the Pytest unit testing framework attests/unit/test_platforms.py
. By using the speed of pytest I was able to increase the number of iterations of the function carried out in a reasonable time, and reduce the probability of flakiness to about 1.6x10^-30 - I think this might be overkill, but it's not slow, and effectively not flaky.Question
Can/Should I delete
tests/flakyfunctional/platform_aliases
?