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

PR: Temporary fix for function test_no_empty_file_items #19617

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

stevetracvc
Copy link
Contributor

looks like the results of findinfiles.find() aren't deterministic and it's causing test_no_empty_file_items to fail

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

Issue(s) Resolved

Fixes #19602

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:
stevetracvc

@dalthviz dalthviz added this to the v5.4.0 milestone Sep 26, 2022
@dalthviz dalthviz changed the title PR: temporary fix for function test_no_empty_file_items PR: Temporary fix for function test_no_empty_file_items Sep 26, 2022
@stevetracvc
Copy link
Contributor Author

What's with this sphinxcontrib error in the MacOS app test?

@dalthviz
Copy link
Member

I think maybe some packages got new releases and those are incompatible with the way the MacOS installer is build. So that error is unrelated with the changes here

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thank you for the help with this @stevetracvc ! This LGTM and the failing check seems unrelated with this 👍

However, just in case, is okay if we merge this without taking into account the failing MacOS installer build check @ccordoba12 ?

@dalthviz dalthviz changed the base branch from master to 5.x September 26, 2022 17:43
@dalthviz dalthviz changed the base branch from 5.x to master September 26, 2022 17:43
@dalthviz
Copy link
Member

Also @stevetracvc could you rebase this change on top of the 5.x branch, please? Thanks!

@stevetracvc
Copy link
Contributor Author

stevetracvc commented Sep 26, 2022

@dalthviz I think I rebased it correctly...I'm not the greatest with git. It still says master at the top. Last time I just redid the PR hah

@dalthviz dalthviz changed the base branch from master to 5.x September 26, 2022 18:17
@dalthviz
Copy link
Member

Thanks @stevetracvc , I think the rebase worked! I changed the base branch on this PR at the top and it still shows just one file as modified 👍

Seems like your git-fu has improved :)

@dalthviz
Copy link
Member

Note: This needs a new rebase on top of 5.x after #19620 gets merged to fix the check failure

looks like the results of findinfiles.find() aren't deterministic
@stevetracvc
Copy link
Contributor Author

Note: This needs a new rebase on top of 5.x after #19620 gets merged to fix the check failure

OK, I think I did that, and the sphinx issue is gone. Is there a better way than me doing

git fetch upstream
git rebase upstream/5.x
git push --force

I don't like the force push part, is there a better way? And I'll need to do it with my other active PR too, once this one gets merged, so that it can pass all tests.

@dalthviz
Copy link
Member

dalthviz commented Sep 27, 2022

Thanks @stevetracvc for your patience with this! And yes that's the way to do it in case you update your PR with a rebase 👍

Regarding the force push, it's needed due to the rebase (which changes the git commit history). The other way to achive synchronization is by merging but then if you already have work done, a merge commit will be put in the history. In such a case, you can just push (so no need of force push since you are just adding the merge commit to the history).

And yes, after this PR gets merged, all the other open PRs will need to either rebase on top the latest 5.x or merge with the latest 5.x to get the changes here and then be able to pass the checks

@dalthviz dalthviz merged commit e562ecf into spyder-ide:5.x Sep 27, 2022
dalthviz added a commit that referenced this pull request Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_no_empty_file_items not working on some CI setups
2 participants