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

Tab and new window support #75

Merged
merged 40 commits into from Mar 31, 2020

Conversation

hgzimmerman
Copy link
Contributor

Still a work in progress at the moment, but figured I should share.

I need the ability to swap tabs for some of the tests I'm writing, and so I've added support here to enable that.

I can attest that the added methods work based on my use of them in my work project, aside from new_window, which I have not had a need for yet.

I intend to contribute a local test suite as discussed earlier as part of this PR, but can't justify that effort at work, so I'll have to do it in my free time.

@hgzimmerman
Copy link
Contributor Author

Hey, I've started to work on a new way of running tests against HTML files served from a local server.

Quick question - I get Session is already started errors when running it as is after the first test in local runs.

The way I got this to work at work was to spawn a new geckodriver instance for each thread instead of having one running in the background. Do you think that is acceptable here?

@jonhoo
Copy link
Owner

jonhoo commented Mar 29, 2020

Ah, yes, it's because for some reason geckodriver doesn't like you driving multiple Firefox instances through one geckodriver instance. I have no idea why. This is why the Firefox tests are all marked as #[serial]. You'll probably want to copy that.

@hgzimmerman
Copy link
Contributor Author

I've updated the geckodriver in the CI setup from v0.24 to v0.26 (which isn't necessarily needed), but apparently Firefox 66 is required for this endpoint to be supported and the CI currently uses 63. [1]

[1] https://github.com/mozilla/geckodriver/releases/tag/v0.24.0

@jonhoo
Copy link
Owner

jonhoo commented Mar 30, 2020

I like this latest change a lot! Leaving some more inline notes.

tests/common.rs Outdated Show resolved Hide resolved
tests/common.rs Outdated Show resolved Hide resolved
tests/local.rs Outdated Show resolved Hide resolved
tests/local.rs Outdated Show resolved Hide resolved
tests/local.rs Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Mar 30, 2020

Looks like you're missing some imports in tests/remote.rs: https://travis-ci.com/github/jonhoo/fantoccini/jobs/309413329#L1232

@hgzimmerman
Copy link
Contributor Author

hgzimmerman commented Mar 31, 2020

Ok, I've added a couple of tests unrelated to the named purpose of this PR to justify the effort of getting that server set up.

I'm happy where this is at currently, let me know if there is anything else you want me to change.

Edit: Thanks a bunch for guiding me through parts of this.

tests/local.rs Outdated Show resolved Hide resolved
tests/local.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Mar 31, 2020

Happy to help! And thanks for sticking with it through all my feedback :) I think the only thing left now is to merge with master and resolve any conflicts, and then we're ready to release it!

@jonhoo
Copy link
Owner

jonhoo commented Mar 31, 2020

Do you agree with the iframe navigation renames? One downside of making them is that the change will require another major version bump, but 🤷‍♂️

@jonhoo
Copy link
Owner

jonhoo commented Mar 31, 2020

Let me know if you have any trouble merging master. I don't think there are any major changes there that should cause you headache.

@hgzimmerman
Copy link
Contributor Author

I think I'm already based off the latest master. You should just be able to merge.

I agree with the naming changes to iframe functions, the fact that new names cause a version bump is your decision to make.

@jonhoo
Copy link
Owner

jonhoo commented Mar 31, 2020

Ah, it looks like GitHub was just confused and thought I wanted to rebase, never mind!

@jonhoo jonhoo merged commit 3568d49 into jonhoo:master Mar 31, 2020
@jonhoo
Copy link
Owner

jonhoo commented Mar 31, 2020

@hgzimmerman I merged the macros in d996234 btw :)

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

2 participants