Skip to content

Conversation

otulach
Copy link
Contributor

@otulach otulach commented Aug 27, 2025

Pull Request Description

Important Notes

Exercise 1

  • Provides full coverage of the first exercise
  • Divided into separate objectives ( as done in the tutorial )

exercise1

@CLAassistant

This comment was marked as off-topic.

@otulach otulach changed the title Dozen new Playwright tests Playwright testing Enso 101 Aug 27, 2025
@enso-bot
Copy link

enso-bot bot commented Aug 27, 2025

Ondrej Tulach reports a new STANDUP for today (2025-08-27):

Progress: Starting with the first exercise and building fundamental infrastructure. It should be finished by 2025-08-29.

@otulach otulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Aug 29, 2025
@enso-bot
Copy link

enso-bot bot commented Aug 29, 2025

Ondrej Tulach reports a new STANDUP for today (2025-08-29):

Progress: Fixing CI issues. Overall restructuring of Playwright test. It should be finished by 2025-08-29.

@enso-bot
Copy link

enso-bot bot commented Sep 1, 2025

Ondrej Tulach reports a new STANDUP for today (2025-09-01):

Progress: Finished first exercise. Diversified into objectives and used functions from previous PR. It should be finished by 2025-09-12.

Copy link
Contributor

@vitvakatu vitvakatu left a comment

Choose a reason for hiding this comment

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

It’s nice to have more tests! I would like to see more unification in testing strategy with existing tests in project-view/widgets.spec.ts, but it can become complicated. If you face difficulties, don’t be afraid to raise questions — a not ideal test is better than no tests at all.

Comment on lines 178 to 184
export async function createNewComponent(page: Page) {
const moreButton = page.getByTestId('more-button').getByRole('button', { name: 'More' }).last()
await expect(moreButton).toBeVisible()
await moreButton.click()

await page.keyboard.press('Enter')
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m a bit confused here. Why do we need to click on the more-button, but then just use Enter to create a node? Also the last() selector seems a bit fragile. I suggest using the same workflow we do in other tests:

  • Find the node you want to use as ‘parent’ using locate.graphNodeByBinding or some other method
  • Click on its grab-handle: .locator('.grab-handle').click() to select it
  • Press Enter to create a new node.

I don’t like choosing nodes by their binding, because it is not obviously visible from graph view at the moment, and it can easily change in the future, but I also don’t like the last() approach. Maybe .last() is enough though!

Copy link
Contributor Author

@otulach otulach Sep 2, 2025

Choose a reason for hiding this comment

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

Yeah the .last() selector is a bit sketchy.
I was using a bit different method for some other components:

  • Found the component trough either .locator() or .getBy or some sort
  • Right click the defining text of that component.
  • Press Enter

Do you think I could unify it to this method?
(Thanks for the review, I will get back to it on friday, once I get back home)

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m still not sure we need to click on the ‘More’ button. Should clicking on a node icon be enough? I even found the graphNodeIcon locator in locate.ts.

// Set the actual filtered number value
const numberBox = page.locator('input.WidgetNumber')
await expect(numberBox).toBeVisible()
await numberBox.fill('3')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we had bugs in the past when using fill instead of typing manually. Here is the excerpt from widgets.spec.ts where we set value of a text input. I suggest using the same approach when editing text fields in these tests as well:

const topLevelArgs = node.locator('.WidgetTopLevelArgument')
const pathArg = topLevelArgs.filter({ has: page.getByText('path') })
const pathDropdown = new DropDownLocator(pathArg)
const pathArgInput = pathArg.getByTestId('widget-text-content')
// Editing text input shows and filters drop down
await pathArgInput.click()
// Using `type` instead of `inputText` here to catch keydown bugs like #13505.
await page.keyboard.type('File 1')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using the keyboard.type method, but kept getting unreasonable issues. So I kept the fill, but unified all the "typing in" into a single function, targeting particular boxes around the widget-text-content as you suggest.

Copy link
Contributor

@somebody1234 somebody1234 left a comment

Choose a reason for hiding this comment

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

CR ❔

@otulach otulach added the CI: Ready to merge This PR is eligible for automatic merge label Sep 7, 2025
@otulach otulach requested a review from somebody1234 September 7, 2025 11:45
@enso-bot
Copy link

enso-bot bot commented Sep 7, 2025

Ondrej Tulach reports a new STANDUP for today (2025-09-07):

Progress: Added new functions and made the test more uderstandable by utilising getByText method. It should be finished by 2025-09-07.

Copy link
Contributor

@vitvakatu vitvakatu left a comment

Choose a reason for hiding this comment

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

I think it is in a good enough state to be merged.

Comment on lines 178 to 184
export async function createNewComponent(page: Page) {
const moreButton = page.getByTestId('more-button').getByRole('button', { name: 'More' }).last()
await expect(moreButton).toBeVisible()
await moreButton.click()

await page.keyboard.press('Enter')
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m still not sure we need to click on the ‘More’ button. Should clicking on a node icon be enough? I even found the graphNodeIcon locator in locate.ts.

/**
* Creating new component from the name of its parent component
*/
export async function createComponentText(page: Page, parentComponent: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call it openComponentBrowser—we are not creating a component; we are just preparing to do so!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely! Everything done.

createNewComponent is still using the 'More' button method, but I am not using this function anymore. Replaced it completely by the newly named openComponentBrowser, which targets the parent components directly. However, there is still the need to keep the original one, due to previous tests.

Comment on lines 96 to 100
await expect(
page
.getByRole('group', { name: TEXT.licenseAgreementCheckbox })
.getByText(TEXT.licenseAgreementCheckbox),
).toBeVisible({ timeout: 60000 })
Copy link
Contributor

Choose a reason for hiding this comment

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

i mentioned this on the first PR but - are you sure we need to wait for toBeVisible?

playwright waits for visibility for clicks:
https://playwright.dev/docs/input#mouse-click

so this should not be necessary on paper. in general playwright prefers doing things as if a human/user were using the app (e.g. roles), rather than via code/programmatic access (e.g. CSS selectors)

Copy link
Contributor

Choose a reason for hiding this comment

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

in this PR it seems there is quite a bit of of boilerplate caused by this, that may be unnecessary due to how playwright works (and if it is necessary, consider maybe writing a helper function do to the visibility check and the click)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you suggest removing waiting toBeVisible in general?
I found few cases where Playwright, even though it should behave like a human, performs an action too fast, which then causes the whole test to crash. So the easiest solution was to add a small wait.

Copy link
Contributor

Choose a reason for hiding this comment

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

nah, toBeVisible would be preferable i think (?) - i think it would be good to verify which specific cases - the most ideal result would be we get a somewhat reliable repro and figure out what's actually causing it

as for a wait: that's unreliable since it will cause flakiness if the runner is too slow

}

/**
* Creating new component from the name of its parent component
Copy link
Contributor

Choose a reason for hiding this comment

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

incorrect docs

}

/**
* Creating new component from the name of its parent component
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: in general, docs for functions should be described like:
"the goal of this function is to ___"

so rather than "creating" or "creates", we should prefer "create"

/**
* Creating new component from the name of its parent component
*/
export async function fillText(page: Page, containerName: string, value: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be fillWidgetText based on its body?

Copy link
Contributor

@somebody1234 somebody1234 left a comment

Choose a reason for hiding this comment

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

CR ✅, nits above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants