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 failing integration tests #978

Merged
merged 1 commit into from Nov 24, 2022
Merged

Conversation

FabioRosado
Copy link
Contributor

Tests in main have been failing since #968 was merged, this PR fixes the failing tests. Looking into it, it seems that the failures are caused by:

  • importing pyscriptjs twice
  • The 'view code' button was blocking the run button so playwright couldn't click it
  • The logo in the navbar was being picked up in the matplotlib

Not sure what's happening yet, but running pytest manually sees to produce different failures than when we run make test-integration-parallel.

These failures weren't also caught by our CI because it's being run only when we have changes to pyscriptjs and the above PR only changed things in the example folder

@madhur-tandon
Copy link
Member

Great find @FabioRosado

One question I have is -- Does the CI still run on changes to pyscriptjs only?
If yes, maybe we should address this in a follow up PR, what say?

@FabioRosado
Copy link
Contributor Author

FabioRosado commented Nov 24, 2022

Thanks! I doesn't because I wasn't sure if we wanted to change CI, but from the discord I think we might haha

@FabioRosado FabioRosado merged commit cafebd6 into pyscript:main Nov 24, 2022
@FabioRosado FabioRosado deleted the fr/fix-tests branch November 24, 2022 09:46
@@ -44,7 +44,6 @@
<link rel="stylesheet" href="./assets/css/examples.css" />
<link rel="stylesheet" href="./assets/prism/prism.css" />
<script defer src="./assets/prism/prism.js"></script>
<script defer src="https://pyscript.net/latest/pyscript.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

uhm... I wonder whether we could/should detect the "double include" case and emit a clean error instead of doing "random things"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I think that's a good idea, I was seeing some weird behaviour on the splash screen saying that the element was already created. I'll open an issue so I don't forget to tackle this 😄

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.

None yet

3 participants