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

Move Assertion Methods #41

Closed
ebebbington opened this issue Jan 9, 2021 · 6 comments
Closed

Move Assertion Methods #41

ebebbington opened this issue Jan 9, 2021 · 6 comments
Labels
Needs Investigation This item needs to be investigated further Type: Chore Merging this pull request results in a patch version increment

Comments

@ebebbington
Copy link
Member

Summary

What:

Move the assertion methods outside of the HeadlessBrowser class as it doesn't really make much sense.

Discuss how we would approach this

Why:

See #39 (comment)

Acceptance Criteria

@ebebbington ebebbington added Type: Chore Merging this pull request results in a patch version increment Priority: Low Needs Investigation This item needs to be investigated further labels Jan 9, 2021
@crookse
Copy link
Member

crookse commented Jan 10, 2021

i think there should be an asserts file for this. that's just my opinion tho

@ebebbington
Copy link
Member Author

But the methods will have to call the existing sendMessage method so they can get the data to assert from

or do you mean create our custom assert?

alternatively, we could just remove the assertions from these methods and leave it down to the user, eg asserUrlIs turns into getUrl

@crookse
Copy link
Member

crookse commented Jan 10, 2021

leaving it up to the user seems like a good idea but let me think about it and try out that UX first

@ebebbington
Copy link
Member Author

Alright sounds good 👍

@ebebbington
Copy link
Member Author

By working on the possibility of adding firefox support, i might have to restructure the methods and classes, and my gut feeling that the end result will address this, with it being:

// Our public API. Holds assertion methods. Holds the `build` logic
class Sinco {}

// everything related to firefox
class FirefoxBrowser {}

// everything related to chrome
cllas ChromeBrowser {}

// How a user woulld use sinco:
const Chrome = await Sinco.buildFor("chrome")
await Chrome.goTo(...)
const Firefox = await Sinco.buildFor("firefox")

@ebebbington
Copy link
Member Author

think we're fine to close this now, only assertsee exists and its on the page class, semtically it makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This item needs to be investigated further Type: Chore Merging this pull request results in a patch version increment
Development

No branches or pull requests

2 participants