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

Fix race in window fullscreen test #5296

Merged
merged 6 commits into from
Dec 8, 2024

Conversation

andydotxyz
Copy link
Member

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

dweymouth
dweymouth previously approved these changes Dec 4, 2024
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like you are passing in a "false" parameter into almost all uses of the function. Would another solution not be better?

@andydotxyz
Copy link
Member Author

I'm not sure what other solution you have in mind.
This helper function was doing too much in one block so either we pass in this extra state or it has to split into separate functions and all the tests have more code than before?

@Jacalz
Copy link
Member

Jacalz commented Dec 4, 2024

If I counted correctly, you are only passing in true in one of the tests. That just makes me think that the parameter shouldn't be there and that that one test needs to be modified, not all the other.

@andydotxyz
Copy link
Member Author

If I counted correctly, you are only passing in true in one of the tests. That just makes me think that the parameter shouldn't be there and that that one test needs to be modified, not all the other.

I understand. Updated.

Trying to get the tests to pass again
@andydotxyz
Copy link
Member Author

I guess the tests don't agree - for some reason that kicked me back to square one.
Off to think more.

@andydotxyz
Copy link
Member Author

Merging in with the requested change to get this release together

@andydotxyz andydotxyz merged commit a31d23e into fyne-io:develop Dec 8, 2024
12 checks passed
@andydotxyz andydotxyz deleted the fix/flakeywindowtest branch December 8, 2024 20:02
@Jacalz
Copy link
Member

Jacalz commented Dec 8, 2024

Off to think more.

I read this as you wanting to work on this more and hence did not review. Sorry if I misunderstood.

andydotxyz added a commit that referenced this pull request Dec 8, 2024
@andydotxyz
Copy link
Member Author

Not a problem. I think the items on GitHub appeared out of order for some reason. The commit above was the result of the thinking.

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.

3 participants