- 
                Notifications
    
You must be signed in to change notification settings  - Fork 0
 
Feat/add internal playwright helpers #78
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
Conversation
| 
           Would it make sense, in addition to the playwright helpers, to export a React component exposing the buttons the helpers rely on? It was somewhat the intent of our shared styled component library, but it did too much for a test app. For instance, we could export something like https://github.com/inrupt/solid-client-notifications-js/blob/7103c3b7650d7b13fb17d361bfd4279b9bb2bd0f/e2e/browser/testApp/components/appContainer/index.tsx#L61, which I already copy-pasted from the notifications test to the access grants one in inrupt/solid-client-access-grants-js#408  | 
    
| 
           Also, it may be out of scope for this PR, so feel free to add a ticket to the backlog for this, but   | 
    
Co-authored-by: Zwifi <nseydoux@inrupt.com>
Co-authored-by: Zwifi <nseydoux@inrupt.com>
          
 I agree that would be useful. @ThisIsMissEm mentioned this as an option as well. I will go ahead and add a ticket for this but do we want it to be under the same JS e2e epic @NSeydoux?  | 
    
| 
           We can add it to the epic, but not tag it for the next release, as it isn't something we strictly need, so we don't want to commit to it on the short term. One of these improve over time things.  | 
    
This package exports testids that are shared between tests fixtures and test apps.
The fixture logs the user in by default, because that's what we want to do most of the time, but also add a flag to disable auto-login for tests requiring not logging in.
| 
           I added a fixture for automated login, see https://github.com/inrupt/solid-client-access-grants-js/pull/407/files for an example usage.  | 
    
Overriding the existing fixture rather than defining a new one allows to not change the existing tests
The previous config resulted in the `index.js` CJS file being overwritten by the `index.js` ESM one.
Keep each page focused on a single view, and move cross-page coordination to the fixtures file.
Create a src/pages directory for pages, and move other files to src/
| "@inrupt/internal-playwright-testids": "^1.4.2", | ||
| "@inrupt/internal-test-env": "^1.4.2" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use the file: links here & npm will resolve it correctly, iirc, that's what we do in the eslint configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting, but is this how it should be done even after these ship? I assumed Lerna was the one taking the wheel here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off top of head, I'm honestly not sure. I just know elsewhere we're using file: links instead of fixed versions & that the publish resolves those to correct versions
Having both auth and startup in the same fixture leads to issues where it's hard to control for a given test whether it is authenticated or not. It is possible to control this behavior at the test suite granularity, but that's not really convenient. Also, the Playright typing system doesn't make it possible/easy to extend the Page class, which means the resulting fixture only has the base page properties, and that doesn't allow higher-level calls like "login" to go through all the login steps.
| "license": "MIT", | ||
| "exports": { | ||
| ".": { | ||
| "require": "./dist/index.js", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| export class CognitoPage { | ||
| page: Page; | ||
| 
               | 
          ||
| static COGNITO_DOMAIN = "amazoncognito.com "; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gut is telling me we shouldnt be hardcoding this, but its consistent across all envs so there is no real reason to right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it's not great, but since the whole concept of pages is pretty much hardcoding what we expect of the pages at least it kinda belongs there I guess
| 
               | 
          ||
| Then add in calls to the functions such as `loginAndAllow` to your test files. | ||
| 
               | 
          ||
| For these helpers to work, your app should use predefined testids defined in | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helpful notes 👍
| 
           LGTM  | 
    
Adding new package called @inrupt/internal-playwright-helpers
Includes reusable pages
CognitoPage,OpenIdPageandTestPageand previously used utility functions such asessUSerLoginthat has been renamed tologinAndAllowfor removing duplicative code across our sdks.