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.

Sign up for GitHub

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

Enable the test to check the added variant #4418

Merged
merged 5 commits into from
Jul 23, 2018
Merged

Enable the test to check the added variant #4418

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
11da7d0
Enable the test to check the added variant
ZheSun88 Jul 19, 2018
ca4ec0b
Adding one more method to the test class
ZheSun88 Jul 20, 2018
a16e17f
Improve
ZheSun88 Jul 20, 2018
1592379
formatting
ZheSun88 Jul 20, 2018
21a6d8f
Javadoc improves
ZheSun88 Jul 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ...ent/flow-component-demo-helpers/src/main/java/com/vaadin/flow/demo/ComponentDemoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ private void verifyThemeIsToggled(List<String> updatedThemes,
previousButtonText, updatedButtonText);

boolean shouldAddTheme = previousButtonText.startsWith("Add");
String[] variant = updatedButtonText.split("'");
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes a strong assumption that the text of the button contains the theme variant.
I think it's very restrictive AND (what's more important) has no any contract.
I would like to have a good contract where I can specify the theme variant explicitly via the method parameter.

This implementation can be kept for existing code with one change: the variants should be checked for lentgh,
there should be a log message which variant is expected and throw if length is not expected.

For any properly written test I would like to use overloaded method with variant explicitly specified as a method parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is not an assumption that the button text contains the theme variant, but it is implemented with demo ( you can check here. 9baee70#diff-fbd0b38f9bdb14592631992c2f7a1effR259).
The variant used here is the split text from add/remove '(theme_variant)' variant, which variant[1] is the theme variant.
As every variant will be a separate button for controlling the toggle, I don't think we need to check the length (you mean the length is the number of variants, right?)
and on the other hand, the theme variant is set via the demo code explicitly, i don't think it is necessary to set here any with the same one.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely an assumption.
You are referring to the method in the DemoView class but this is a method in the ComponentDemoTest class.
Those two classes has no any relation to each other.
As I said already many times: there is no contract at all (well defined or not , no ANY contract at all).

Nothing prevents me calling the method verifyThemeVariantsBeingToggled without even using DemoView class in my UI class at all.
Do I have to use DemoView ? I don't. You can't force me to use it.
It means you have no any contract.
So if I've created a button which somehow follows some expectation of verifyThemeVariantsBeingToggled but with totally arbitrary text then it fails without giving me a reason why and what's going on.
This is very bad coding practice.
The main reason why we are using Java: it allows to define clear contract which FOCRES to follow the expectation of a developer.
The way of programming where assumptions without declaring exact contract expressed in language is bad and possible in any language. There is no any point to use Java if the same unclear magic code can be written in JS, C, C++.

I need contract.
That's what I want from the method.
It should not assume anything.
I should be able to give the exact theme variant which is expected to be present or absent.
In this way I may read the code locally (only one method) and don't have to "grep" all classes which are expected to be used if you want to use this test method.
At the moment this test looks magically. I don't understand why it works.

Ideally this method should accept also locators for button and component (because this is also some kind of magic: I don't see from the method call what is the context for it). But this I can leave with.
Button text can be arbitrary and this really bothers me.

Copy link
Contributor

Choose a reason for hiding this comment

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

So what do you propose that could be made here @denis-anisimov ? The main problem is that the "communication layer" between the logic and the test is done by a text in the UI (the updatedButtonText in this case) - every time and variant is added or removed, that text is updated and the test reads that to evaluate what variants should be present in the page.

Nothing prevents you from calling this method for a test that doesn't use a DemoView as the view implementation on the server side. But how could it be prevented? There's no way for the IT to access the server class that is being executed for the test AFAIK, so the "message exchange" is done via some element in the page. This has been done in several different tests, but in this case there's some parsing involved and some obscure logic that is probably hard to maintain, but I don't know how could it be improved other than putting some comments explaining how the mechanism work.

Copy link
Contributor

Choose a reason for hiding this comment

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

The proposition is already written in the comments many times.
It might be it will require too many changes since the current implementation doesn't accept any context but
goes through every button by the hardcoded locator. Since there are many buttons my proposition cannot be
implemented in the straightforward way.
This code smells but I'm tired to explain why.
This method should have not ever been added to use as an utility method. But I'm not able to review every commit so it's to late to cry about it.
I give up since it's just a test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, now I get it, you're complaining not just about this particular change but about the overall design of the solution. Sorry I also didn't review this design so I have no idea where are your comments about it.

I suggest that a ticket is created to address this test in general, with a better design to solve the problems and provide the functionality that is needed. At least then we can discuss better with the big picture in mind, not with just point changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

A half minute of thinking:
the verifyThemeVariantsBeingToggled method needs one more method:
verifyThemeVariantsBeingToggled(Function<WebElement,String> variantProducer).
variantProducer is a parameter which is either implemented by the developer who is using the method OR
there should be extracted separate producer class (which javadocs) from this implementation :

public apply(WebElement button){
String[] variant = button.getText().split("'");
return variant[1];
}

verifyThemeVariantsBeingToggled implementation just calls verifyThemeVariantsBeingToggled(defaultProducer).

Now I'm blocking this because the implementation sketch is clear and written here.
This way the contract is clearly defined: I see how button gives the variant which is expected to be present or absent.

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 am not familiar with this implementation, but just take a try.
Is this what you wanted?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but please improve the code as I suggested and everything will be fine.

if (shouldAddTheme) {
Assert.assertTrue(
"When a theme variant got added, toggle button text should start with 'Remove' word",
Expand All @@ -115,6 +116,11 @@ private void verifyThemeIsToggled(List<String> updatedThemes,
Assert.assertTrue(
"When a theme variant got added, component 'theme' attribute should contain all previous theme variants",
updatedThemes.containsAll(previousThemes));

Assert.assertTrue(
"The selected theme variant:" + variant[1]
+ " should be added to the component 'theme' attribute.",
updatedThemes.contains(variant[1]));
} else {
Assert.assertTrue(
"When a theme variant got removed, toggle button text should start with 'Add' word",
Expand All @@ -125,6 +131,10 @@ private void verifyThemeIsToggled(List<String> updatedThemes,
Assert.assertTrue(
"When a theme variant got removed, previous theme variants should contain all theme variants from component 'theme' attribute",
previousThemes.containsAll(updatedThemes));
Assert.assertFalse(
"The selected theme variant:" + variant[1]
+ " should be removed from the component 'theme' attribute.",
updatedThemes.contains(variant[1]));
}
}

Expand Down