-
Notifications
You must be signed in to change notification settings - Fork 14
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
test: create colony flow #3327
test: create colony flow #3327
Conversation
This shouldn't be based on the So the base branch should be |
b936e41
to
ff1c455
Compare
ffb0989
to
cbdf36a
Compare
@olgapod please assing yourself to PRs |
cbdf36a
to
dcfeb30
Compare
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.
All the tests fail for me here, I assume it's something to do with playwright's setup as it seems to not be starting the dev env.
Either that, or your testing instructions are missing a few steps.
Speaking of the above, until everybody gets comfortable with this, please also include the steps to actually set up playwright (install, etc).
Basically what I am saying is be more comprehensive when writing your testing instructions.
@rdig I missed the step to start the dev environment (BE services) before running the tests, added it 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.
Ran this with the dev environment up and running and while some initial tests work, most of them fail or time out due to your usage of a hard coded token address.
Token addresses are not static, so you cannot hard code them.
Also, please leave a comment on this PR with what I told you in our private chat, as to why you're not wiring up the dev environment to run automatically, but have to start it separately by hand.
package.json
Outdated
@@ -38,7 +38,7 @@ | |||
"postinstall": "if [ -z \"$SKIP_HOOKS\" ]; then ./scripts/lambda-functions-dependencies.sh && scripts/generate-amplify-local-config.sh; fi", | |||
"watch-amplify": "node scripts/watchAmplifyFiles", | |||
"forward-time": "node scripts/forward-time", | |||
"playwright:install": "npx playwright install --with-deps", | |||
"playwright:install": "npx playwright install --with-deps", | |||
"playwright:run": "playwright test", |
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.
can you please change this to something like "test" or "execute" or something, just having npm run playright:run
is very redundant :)
Not a major gripe but a nitpick here
playwright/e2e/create-colony.spec.ts
Outdated
test.describe('Create Colony flow', () => { | ||
let colonyUrl: string; | ||
const colonyName = 'testcolonyname'; | ||
const validToken = '0xeF841fe1611ce41bFCf0265097EFaf50486F5111'; |
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.
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.
Interesting problem to solve. Even if we can get away with manually creating a token in the DB for now, will the tests have to interact with the chain like create-data script does?
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.
The proper solution here is to use one of the "verified" tokens we deploy via the create data script and fetch it's address, either from the db, or from the chain directly
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.
thanks, I've used your advice and updated the PR
playwright/e2e/create-colony.spec.ts
Outdated
}); | ||
}); | ||
|
||
test('Should render Confirmation step as expected', async ({ page }) => { |
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.
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.
updated, removed hardcoding the token address
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.
Great to see a start on the create colony flow, very nice work @olgapod 🎉
Unfortunately I'm also seeing some tests fail:
playwright/e2e/create-colony.spec.ts
Outdated
test.describe('Create Colony flow', () => { | ||
let colonyUrl: string; | ||
const colonyName = 'testcolonyname'; | ||
const validToken = '0xeF841fe1611ce41bFCf0265097EFaf50486F5111'; |
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.
Interesting problem to solve. Even if we can get away with manually creating a token in the DB for now, will the tests have to interact with the chain like create-data script does?
1504d58
to
16d4a44
Compare
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.
@jakubcolony I'll add that test case in the next PR. Good catch, thanks! Also, please feel free to suggest test cases going forward, it would be helpful as I'm still learning all the features of the app |
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.
Great work on this, all working nicely and all tests are passing.
As Jakub mentioned, it would be good to test for invalid but properly formatted addresses.
It might be worth checking src/components/common/Onboarding/wizardSteps/CreateColony/validation.ts
for any other relevant validation checks. For example, there is a list of RESERVED_ROUTES and it shouldn't be possible to set your colony url to any of these reserved routes. I think we should get this merged and add these in a later PR though. :)
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.
This is getting closer. Sorry I keep finding changes and preventing you from merging this already :(
First of, on my machine, tests fail in headless mode, which might be a "me" issue. So we'll need some confirmations from other people
(I'm running this commit)
However, the UI run of the tests seem to be running fine
I've also left some comments on what other checks you need to add to make this PR complete.
Sorry again, for holding this back still.
playwright/e2e/create-colony.spec.ts
Outdated
await page.getByLabel(/token symbol/i).fill(tokenSymbol); | ||
}); | ||
|
||
test('Token Logo file uploader should work correctly', async ({ page }) => { |
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.
Please also test going back and forth and ensure the token name, symbol and logo persist
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.
Basically testing the bug you identified here: #3300
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.
added for the token name and symbol. for the logo I will add a test once the bug is fixed
playwright/e2e/create-colony.spec.ts
Outdated
); | ||
}); | ||
|
||
test('Should create a colony and navigate to the Complete Setup step', async ({ |
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.
At this stage the colony is not yet created, so this description is a bit misleading.
I would actually have this step wait until all transactions are complete and you get the "successfully created colony" welcome modal (although that might be harder)
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.
added this step, although it's quite slow as the transactions and transition to the new colony page take some time
).toBeHidden(); | ||
}); | ||
|
||
test('Should accept existing token and display validation message', async ({ |
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.
Can we have a test where we also check invalid tokens. It should check:
- a bad formatted address
- a good formatted address, which is a user address
- a good formatted address, which is a colony address
- a good formatted address, but one that doesn't exist on the chain
- address zero (it's a good formatted address, exists on the chain, but you shouldn't be allowed to add it as a native token)
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.
done
Interesting, Webkit tests are failing on your machine for some reason. To identify if it's your env specific problem or an issue with the tests please try on a clean Playwright example proj by running this command in your terminal |
👍 |
343ac17
to
c08b5bb
Compare
de81248
to
f7f7f53
Compare
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.
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.
playwright/utils/create-colony.ts
Outdated
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 would suggest creating a helper function for getting an input field by label and also add the { force: true }
option - without this option I believe the Continue
button will not get enabled, but would love to hear your thoughts
Also, can we please create a constants object holding all the text strings that we are using for getting a text field/button etc?
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.
Thanks for your suggestions! @mmioana
-
I don't think creating such a helper would make the code significantly shorter or easier to read.
-
Regarding the use of
{ force: true }
, I'm not sure I fully understand the need for it. This option is used to bypass Playwright's actionability checks, like when an element is hidden or disabled. In our case, are we looking to click buttons regardless of their, for example, disabled state? -
Creating a constants object to hold all the text strings for selectors - sure we can do this. would be interesting to see what others think about this
@olgapod please set up a call w/ me, @iamsamgibbs (and however else is interested) so that we can debug this live, as I'm experiencing the same as Sam, but @jakubcolony 's seem to work. It's either a OS thing, or a resource thing, but either way the tests seem flaky at the moment. So let's jump on a call and debug this live, as I want to get this merged as soon as possible |
hey guys, these are errors that I get :) @olgapod I am also interested in investigating this, please include me in a call as well 🙌 |
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.
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.
Hey @olgapod ! I had a go at this and tried my best to explain the issue I'm having with one of the tests. But if it consistently passes for everyone else including you, don't mind my comment and I'll just approve it 👌 😄
for (const value of values) { | ||
await inputField.fill(value); | ||
|
||
await expect(page.getByTestId('form-error')).toBeVisible(); |
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.
Hey @olgapod awesome job on this PR! I ran this locally and the test seems to consistently fail on this step, as in like 10 times in a row:
Looking at the recorded timeline, it looked like it was entering the 21-length string (A.repeat(21)
) into the field which is correct and expected but for whatever reason, the input gets cleared afterwards, which causes this test to fail.
I experimented a little bit and found that the problem went away after adding a 100ms timeout to the fill function:
// I really don't know why.... 🤷🏻♂️
await inputField.fill(value, { timeout: 100 });
There are however also other instances where this test would just pass without this shenanigan 😅
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.
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.
EDIT:
@olgapod Both tests have passed 5 out of 5 times on my machine without any code changes whatsoever 🚀
|
||
for (const value of values) { | ||
await inputField.fill(value); | ||
await inputField.blur(); |
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 the test will be a more accurate real-world representation if you didn't blur the field here because our validation is triggered via onChange 👌
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.
Hey @olgapod it seems that CI is having issues with these as well 😕 I thought it was just my machine. I had some luck "fixing" these yesterday on my local, I outlined it on my comment yesterday. No promises it works on CI though, but I guess it's worth a shot? 🤞 |
Thanks all for your time to debug this. The most frustrating part is that I can't reproduce the failures on any of my three laptops (although all macOS based, but with very different specs). For now, my best idea is to close this PR and break down its changes into smaller ones - one for each step of the flow. I think this will help pinpoint the problem with flakiness |
Description
Testing
Prerequisites: The test script simulates user interactions with the local instance of the web app, which means that the BE is expected to be set up and running. For now, backend services should be started manually and the initial test data should be seeded . Therefore, ensure that the development environment is properly set up by following the instructions in the README file
Setup and start your local dev environment (all BE services should be up and running)
npm run dev
Seed the data, if it's not already done before
node scripts/create-data.js
If it's the first time you run e2e tests on your machine - run
npm e2e:install
run
npm run e2e
ornpm run e2e:ui
Ensure all tests pass
Diffs
New stuff ✨