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

Run Edge in app mode #612

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

dstricks
Copy link
Contributor

This is to address #605

@samuelhwilliams
Copy link
Collaborator

It would be great to get the windows tests passing so that we maybe had more confidence that this was a valid change. But I don't think they've ever been passing unfortunately.

I'll check this out on my PC in a bit and confirm that it's work.

@dstricks
Copy link
Contributor Author

dstricks commented Oct 1, 2022

It would be great to get the windows tests passing so that we maybe had more confidence that this was a valid change. But I don't think they've ever been passing unfortunately.

Agreed. I think I saw you had a branch from a while ago where you were trying to get it to behave. I'll pick up that effort eventually :)

Can we disable these automated Windows checks until they're fixed? I don't want anyone to get in the habit of seeing failed stuff and then casually thinking it's the "Windows tests that have always failed." IMHO, everything should pass when people contribute code and they shouldn't be desensitized to the failures. And then when the Windows checks are running and stable, we add them back in?

I'll check this out on my PC in a bit and confirm that it's work.

For this particular PR, I did setup the environment and had the Tox tests running on my Windows machine, but I was turned off on the ChromeDriver experience that I had. And things seemed to be a bit more... flaky and I couldn't determine exactly why. So I switched over to Linux just today and everything was smooth and worked as advertised. The Windows/Tox/ChromeDriver problems could totally be my fault... but it left a bad taste.

That said, I did run the 01 - hello_world-Edge/ manually on my Windows box and confirmed it was working properly.

Lastly... I know I'm mixing PR and issue threads here, but if you had any comments on this question that I asked:

A related design question... is there any reason to preserve the ability to open up content in a new tab (which might suggest a flag or similar is needed)? I can't think of one... does anyone else have a valid use case?

@samuelhwilliams
Copy link
Collaborator

Can we disable these automated Windows checks until they're fixed? [...] And then when the Windows checks are running and stable, we add them back in?

I could go either way here. I agree it's not good to get people used to expecting tests to fail, but it's also a constant reminder to us (now maybe you) that we should get our act together and fix the Windows tests. Whereas if we disable them and hide them, it might be easier to forget. Whichever way you want to go is probably fine - just remove windows-latest from here: https://github.com/python-eel/Eel/blob/master/.github/workflows/test.yml#L14

The Windows/Tox/ChromeDriver problems could totally be my fault... but it left a bad taste.

I think that's broadly my memory of it as well :(

Lastly... I know I'm mixing PR and issue threads here, but if you had any comments on this question that I asked:

Will take a look at the issue.

@doug-benn
Copy link
Contributor

doug-benn commented Oct 18, 2022

Hello,

I'll be honest I'm new to test's but I saw this and decided to have a go. With some work I got them to run on Windows not sure how it works with the different python version & github actions etc. but they ran with no error on 3.10. I can only assume I am doing something wrong if they aren't working?

The Windows/Tox/ChromeDriver problems could totally be my fault... but it left a bad taste.

Not your fault at all! Not sure I like it, maybe the driver could be installed as part of the test? (I might try and work this out myself now I know the tests pass)
Edit: I have just looked and I think the ChromeDriver is the main thing causing the fails?

Edit 2: Nevermind the Windows tests make me sad and I can't get them to run with tox, got the error's down to eel not being imported for some reason

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.

3 participants