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

Extend assertNotExist with a new implementation using a viewMatcher as a param #479

Merged
merged 4 commits into from
Jan 12, 2023

Conversation

moutyque
Copy link
Contributor

@moutyque moutyque commented Jan 3, 2023

No description provided.

@rocboronat
Copy link
Member

Hello @moutyque 👋

Thanks for the PR! It looks good to me, although I'm a bit disconnected from the project, so I would like to have another reviewer here, maybe @alorma :)

One thing, could you also add it to the readme?

Thank you so much!

@rocboronat rocboronat requested a review from alorma January 3, 2023 13:52
@rocboronat
Copy link
Member

By the way, checking the current readme we have, if we start implementing things like this, we should also add the rest of matchers, right? I mean assertExists, assertVisible... etc...

@moutyque
Copy link
Contributor Author

moutyque commented Jan 4, 2023

Hello @rocboronat ,
Thx for your quick feedback.
About implementing other method as well, I do believe it is a good idea however I would like to do that in different PR to avoid being stuck for too long. Doing one function per PR seems to me a better way to do it because we could provide new features over the time and not having to wait a big batch to release them all.
Please give me your feedback.

@moutyque
Copy link
Contributor Author

moutyque commented Jan 4, 2023

Also I run locally the step that failed with the CI: ./gradlew :sample:connectedCheck :barista-compose:connectedCheck
and it ends properly. Could you advice how to fix the CI ?

Copy link
Member

@alorma alorma left a comment

Choose a reason for hiding this comment

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

Hi @moutyque! Sorry for the delay on my review. I was out of laptop for holidays.

Thanks for this PR. I think it could be very useful!

Btw, as I agree with @rocboronat on other methods, I will suggest to create at least the assertExists variant also, so we have both positive and negative versions. Later another PR for other kind of methods.

What do you think?

@moutyque
Copy link
Contributor Author

moutyque commented Jan 4, 2023

Hello @alorma ,
I fact I though assertExist already exist but no.
I agree with you, I will add the positive assertion.

@moutyque
Copy link
Contributor Author

@alorma can you check again.
I add assertExist

@alorma alorma enabled auto-merge (squash) January 12, 2023 11:48
@alorma alorma merged commit 602f148 into AdevintaSpain:master Jan 12, 2023
@moutyque moutyque deleted the 470 branch March 3, 2023 20:24
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.

4 participants