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.

Sign up for GitHub

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

Jump to bottom

feature #79 - recording support #80

Merged
merged 19 commits into from
Nov 25, 2024
Merged

feature #79 - recording support #80

merged 19 commits into from
Nov 25, 2024

Conversation

jdaugherty
Copy link
Contributor

Adds initial recording support.

sbglasius
sbglasius reviewed Nov 23, 2024
View reviewed changes
Copy link
Contributor

@sbglasius sbglasius left a comment

Choose a reason for hiding this comment

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

See code comments. Overall a solid piece of work. Thanks

src/testFixtures/groovy/grails/plugin/geb/ContainerGebSpec.groovy Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebRecordingExtension.groovy Outdated Show resolved Hide resolved
@fernando88to
Copy link

This functionality could be in Grails 6, as well as the service per container.

@jdaugherty
Copy link
Contributor Author

This functionality could be in Grails 6, as well as the service per container.

We plan to back port it. When you say a service per container, can you please clarify?

@fernando88to
Copy link

fernando88to commented Nov 24, 2024 •
edited
Loading

I mean the ContainerGebSpec service #61 however there is already an issue #79

matrei
matrei requested changes Nov 24, 2024
View reviewed changes
Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

Looks good! Many minor comments and one big one (Optional.ofNullable)

src/testFixtures/groovy/grails/plugin/geb/ContainerGebConfiguration.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebConfiguration.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebConfiguration.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebConfiguration.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebConfiguration.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebTestDescription.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebTestDescription.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebTestListener.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebTestListener.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebConfiguration.groovy Outdated Show resolved Hide resolved
@jdaugherty
Copy link
Contributor Author

I've gone ahead and moved all of the container lifecycle management to the spock extension. I also utilize a jvm shutdown hook to ensure the containers are always terminated - so force killing the test run won't leave containers running now.

matrei
matrei reviewed Nov 24, 2024
View reviewed changes
Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

Nice! Some more comments.

src/testFixtures/groovy/grails/plugin/geb/ContainerGebSpec.groovy Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebTestDescription.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebTestListener.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/RecordingSettings.groovy Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/RecordingSettings.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebRecordingExtension.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebRecordingExtension.groovy Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/ContainerGebRecordingExtension.groovy Outdated Show resolved Hide resolved
src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy Outdated Show resolved Hide resolved
@jdaugherty jdaugherty requested a review from matrei November 24, 2024 19:53
@jdaugherty jdaugherty requested a review from matrei November 24, 2024 20:45
@jdaugherty jdaugherty requested a review from matrei November 24, 2024 21:04
matrei
matrei approved these changes Nov 24, 2024
View reviewed changes
Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

Great work James!
I have not tested the new annotation. Maybe @sbglasius can do that as he had some use cases.

@jdaugherty
Copy link
Contributor Author

Thanks @matrei . I attempted to start back porting these changes but ran into a blocker: I documented the issue in #73.

I'll merge this PR once @sbglasius gives the OK.

@sbglasius
WebDriverContainerHolder getHostIp did not get Host ip, but container IP
a36bd7e
@sbglasius
Suggesting a test-app for the basic tests.
895466b 
* Not auto-imported
* Stand-alone project
* Includes 'geb' as subproject (not the other way around)
* Project has grails.geb.recording.mode = RECORD_ALL in build.gradle
@sbglasius
Copy link
Contributor

My use-case does not work, but I have provided a PR for @jdaugherty's branch.

sbglasius
sbglasius requested changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@sbglasius sbglasius left a comment

Choose a reason for hiding this comment

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

Solid piece of work @jdaugherty - we're almost there!

src/testFixtures/groovy/grails/plugin/geb/WebDriverContainerHolder.groovy Outdated Show resolved Hide resolved
@jdaugherty
Copy link
Contributor Author

Thank you for the work! I merged your changes into my branch.

sbglasius
sbglasius approved these changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@sbglasius sbglasius left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jdaugherty jdaugherty merged commit 8824159 into grails:5.0.x Nov 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbglasius sbglasius sbglasius approved these changes

@matrei matrei Awaiting requested review from matrei

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jdaugherty @fernando88to @sbglasius @matrei