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

Set current time as test time in setup #434

Conversation

rautamik
Copy link
Contributor

@rautamik rautamik commented Nov 1, 2024

Bugfix for #401

Changes:
Set current time as test time in setup

@rautamik
Copy link
Contributor Author

rautamik commented Nov 1, 2024

Hi @lanedirt,

I noticed that you reopened flaky espionage dispatch mission test and thought that maybe we could simply set the current time in test setup to get these timing related flakies fixed. I have run the espionage dispatch mission test locally in loop with this fix and wasn't able to reproduce it anymore.

There is another problematic flaky test, testDispatchFleetCombatUnitsLostAndResourceGained, still randomly failing like while testing this PR.

Sorry to jump into something that you are already looking into, just came with an idea and wanted to see if it works...

@lanedirt
Copy link
Owner

lanedirt commented Nov 1, 2024

Hi @rautamik,

Thanks for also looking into this, more eyes is alway better 😄 !

I have already made a new PR #433 that includes the same as what you're proposing but with more tweaks to all existing tests. Now in the main account testcase the time is set statically to 2024-01-01 and individual tests simply advance time by calling $this->travel() which is a Laravel wrapper that works with Carbon. There were also some issues I noticed that (sometimes) occur when tests are ran against an empty database which has to do with setting/changing server settings.

All in all I think that PR #433 should address all issues and that we can close this PR as duplicate. Do you agree?

@rautamik rautamik closed this Nov 1, 2024
@rautamik
Copy link
Contributor Author

rautamik commented Nov 1, 2024

Yes, I agree that this is a duplicate and can be closed.

Nice to see that you have refactored lot of test cases, hopefully all flaky ones are now fixed. I was also thinking that travel would be more readable to use.

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