Skip to content

Pin a version of py.test to see if it fixes the Travis CI crash #1057

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

Closed
wants to merge 6 commits into from

Conversation

tjguk
Copy link
Collaborator

@tjguk tjguk commented Jun 11, 2020

No description provided.

@tjguk
Copy link
Collaborator Author

tjguk commented Jun 11, 2020

That didn't work. I'm fairly unfamiliar with Travis.

@carlosperate -- looks like you were the last one here, adding back 3.5 support which @ntoll took out earlier (presumably for a good reason ;) ). Since we're looking to retarget to at least 3.7 (f-strings?) can we drop 3.5 now altogether? Or is there something else going on here...?

@carlosperate
Copy link
Member

The main reason to maintain Python 3.5 compatibility was that it was the version available via the default apt repositories in Debian and Raspbian Stretch.

With the Buster release almost a year ago I guess it would be reasonable to drop 3.5 support.
However, if we are doing a release with a lot of bug fixes that could still be affecting Stretch users maybe it will be good to make one last release compatible with Stretch? I don't really have any strong feelings about this.

What would be the main disadvantage of continue testing in 3.5 as it is right now? The current issue in Travis seems to be happening in all Python versions? We could start using f-strings for any updates after the next release?

@tjguk
Copy link
Collaborator Author

tjguk commented Jun 11, 2020

Thanks for the explanation. Re-reading my comment, I'd intended the "for a good reason" comment to apply to your reversal, but I see now that it read more like an emphasis on the original removal. Sorry about that :(

In short: happy to keep it in there for my purposes. It's just that, on a quick inspection of the error logs we seemed to be bailing out on a 3.5 check -- and if removing 3.5 was an option it would be one less thing...

@carlosperate
Copy link
Member

No worries, it's good to clarify we still support older versions.

If there was something that was not working on 3.5 and required an ugly workaround or extra work to get running then I would probably vote to remove support (so I was assuming the current issues we have in Travis are not directly related to 3.5). The main takeaway from my comment was that if there is no extra effort to keep it then we might as well maintain it until the next release.

@tjguk
Copy link
Collaborator Author

tjguk commented Jun 11, 2020

Now I think about it, one issue is that Black only supports 3.6+ and the hope was to include the [Tidy] button in this release. I'll leave it to @ntoll to make a call, though. My only immediate issue -- and it's not that immediate -- is that the Travis CI is failing and I was looking to simplify, if only so it failed quicker!

@carlosperate
Copy link
Member

If I remember correctly the Tidy button was only displayed if the black package was importable, so it was already designed with the idea that users in 3.5 would not have the button in their UI.
So the packaged version of Python should work as expected with black bundled, and 3.5 users can still run Mu without that feature.

@carlosperate
Copy link
Member

carlosperate commented Jun 11, 2020

Ups, button is displayed on Python 3.6 or newer, independently of black being importable, so I didn't remember it correctly, but close enough 😅

mu/mu/interface/main.py

Lines 152 to 157 in b341cec

if sys.version_info[:2] >= (3, 6):
self.addAction(
name="tidy",
display_name=_("Tidy"),
tool_text=_("Tidy up the layout of your code."),
)

mu/mu/logic.py

Lines 1608 to 1618 in b341cec

def tidy_code(self):
"""
Prettify code with Black.
"""
tab = self._view.current_tab
if not tab or sys.version_info[:2] < (3, 6):
return
# Only works on Python, so abort.
if tab.path and not tab.path.endswith(".py"):
return
from black import format_str, FileMode, PY36_VERSIONS

@tjguk
Copy link
Collaborator Author

tjguk commented Jun 11, 2020

Ah. I did wonder whether that could be / was in place. Good. So let's leave the 3.5 thing alone for now. Thanks for following up :)

@dybber
Copy link
Collaborator

dybber commented Jun 12, 2020

It seems the error occur during make coverage stage. What if you also pin pytest-cov to the previous version, e.g. pytest-cov==2.8.1? or maybe pytest-cov==2.7.1

I'm just guessing here, it's hard to figure out, when it doesn't happen for us locally.

@dybber
Copy link
Collaborator

dybber commented Jun 12, 2020

Next idea. The error seems to happen when calling the QApplication constructor in test_dialogs.py. Perhaps it is no longer necessary? At least the test cases works without that line on my machine.
https://github.com/mu-editor/mu/blob/master/tests/interface/test_dialogs.py#L14-L17

If it is necessary, I still think such initialization would be better to place in pytest fixture, rather than initialized when the test-module is loaded. Then the error would be reported at the test case that fails, rather than abort the whole test-process (I believe). The dependencies would also be made clear, currently the code doesn't tell which test cases really depends on this line to be there.

@tjguk
Copy link
Collaborator Author

tjguk commented Jun 12, 2020

Thanks for persevering :) I'll give it a try...

@dybber
Copy link
Collaborator

dybber commented Jun 12, 2020

Same issue, now in test_panes.py it seems ;-)

@tjguk
Copy link
Collaborator Author

tjguk commented Jun 12, 2020

At least we've kicked the bump along the carpet a bit. I'll give that a kick later on...

@tjguk
Copy link
Collaborator Author

tjguk commented Jun 12, 2020

Or @dybber you're very welcome to start a new PR and try various things out. I'm not tied to this one :)

@tjguk
Copy link
Collaborator Author

tjguk commented Jun 12, 2020

Great: now it's failing because of the flake8 grumbles about unused imports. Here we go...

@tjguk
Copy link
Collaborator Author

tjguk commented Jun 12, 2020

Still failling. Coverage just bombs out (even locally) at test_app only for this branch, so presumably there's an incompatibility somewhere.

I'll try to play around with versions a little...

@dybber
Copy link
Collaborator

dybber commented Jun 12, 2020

Perhaps the version pinning wasn’t necessary after all.

And yeah I thought about creating a second PR and play around. Let me know if you want me to look at it, I have some time for it this evening as well as tomorrow.

@tjguk
Copy link
Collaborator Author

tjguk commented Jun 12, 2020

Long and short: py.test itself is failing on this branch. Master is fine. If you run py.test --verbose you'll see it fail on, eg, test_run. If you use --random-order it'll fail elsewhere. Haven't dived in yet, but probably worth spawning a fresh PR from master with nothing changed and seeing what it takes to fail

@dybber
Copy link
Collaborator

dybber commented Jun 12, 2020

Okay, I'll give it a try!

@tjguk
Copy link
Collaborator Author

tjguk commented Jun 13, 2020

Superseded by #1065

@tjguk tjguk closed this Jun 13, 2020
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