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

Write up Playwright code style. #53170

Closed
worldomonation opened this issue May 26, 2021 · 5 comments · Fixed by #53354, #58610 or #59017
Closed

Write up Playwright code style. #53170

worldomonation opened this issue May 26, 2021 · 5 comments · Fixed by #53354, #58610 or #59017
Labels
[Pri] Low Address when resources are available. [Type] Task

Comments

@worldomonation
Copy link
Contributor

Task Details
There is enough difference between the Playwright and Selenium frameworks that I think an update to the code style (including naming variables, etc) should be updated.

Related to
Parent: #52464

@worldomonation
Copy link
Contributor Author

In the next round of documentation update, include the following:

  • use of separate type dependencies field at top of file
  • use of object parameter where possible
  • having minimal steps where possible
  • prefer explicit action over implicit (eg. likePost instead of clickLikePost)

@WunderBart
Copy link
Member

WunderBart commented Jun 17, 2021

use of object parameter where possible

IMO, this is a good practice where the method name itself doesn't suggest the argument type/value. Here's an example where that applies (taken from some recent refactor):

BAD

await GutenbergEditorComponent.publish( true );

It's not immediately obvious what we're enabling here when passing true. So instead what we did, was:

GOOD

await GutenbergEditorComponent.publish( { visit: true } );

Now we know that most probably, we're going to visit the published post/page.

I don't think this would be good as a general rule as many times it's unnecessary or even unintuitive. For example, from this recent PR - MediaPage.clickOn( { item: <itemIndex> } ) looks a bit forced to me, probably due to this convention being applied. If it's a single-purpose function that clicks an item with the given index, IMO it's fine to just do MediaPage.clickItem( index ) (or even MediaPage.selectItem( index ) according to the last point from the list above). Does that make sense?

@worldomonation
Copy link
Contributor Author

@WunderBart I understand what you are saying and I think it's reasonable.

I am not sure how we should codify this however. I suppose I can write this up like the following (paraphrased):

If function name suggests the purpose, or if the parameter is a single purpose, do not use a named object parameter.
Otherwise, use a named object parameter.

<insert examples as you have shown here>

Or do you think that perhaps it's better off not including guidance on this at all?

@WunderBart
Copy link
Member

Or do you think that perhaps it's better off not including guidance on this at all?

I think so, yeah. It's more of a general rule than one that would apply to the E2E code specifically. Let's skip it 👍

@WunderBart
Copy link
Member

WunderBart commented Jun 24, 2021

prefer explicit action over implicit (eg. likePost instead of clickLikePost)

I think this is something that should depend on the context. I.e. for a page model, I'd use the LoginPage.clickLoginButton() because it defines a specific action for an element of that page. On the other hand, when defining a flow action, we're looking from the user's perspective, so I'd go with LoginFlow.submitLogin(). Does that make sense?

Here's a wider example of this convention that made me realize the difference between page/screen & flow models (from https://youtu.be/-oPYQL42pGs?t=1752):

Screenshot 2021-06-24 at 13 52 50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Pri] Low Address when resources are available. [Type] Task
Projects
None yet
2 participants