Enable the test to check the added variant #4418
Conversation
| @@ -105,6 +105,7 @@ private void verifyThemeIsToggled(List<String> updatedThemes, | |||
| previousButtonText, updatedButtonText); | |||
|
|
|||
| boolean shouldAddTheme = previousButtonText.startsWith("Add"); | |||
| String[] variant = updatedButtonText.split("'"); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I am not familiar with this implementation, but just take a try.
Is this what you wanted?
There was a problem hiding this comment.
yes, but please improve the code as I suggested and everything will be fine.
| @@ -105,6 +105,7 @@ private void verifyThemeIsToggled(List<String> updatedThemes, | |||
| previousButtonText, updatedButtonText); | |||
|
|
|||
| boolean shouldAddTheme = previousButtonText.startsWith("Add"); | |||
| String[] variant = updatedButtonText.split("'"); | |||
There was a problem hiding this comment.
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.
| * With current design, the theme variant can be obtained from the button | ||
| * attached to the demo | ||
| */ | ||
| private Function<WebElement, String> defaultProducer = ( |
There was a problem hiding this comment.
Please make this class visible so it can be reused/extended:
- make the class nested (not anonymous) (and not inner, it should be static).
- make a static constant
DEFAULT_VARIANT_PRODUCER. - set
DEFAULT_VARIANT_PRODUCERto the instance of the created class - pass it as an argument to the
verifyThemeVariantsBeingToggledin the above impl.
| @@ -56,31 +56,52 @@ public void openDemoPageAndCheckForErrors() { | |||
| } | |||
|
|
|||
| /** | |||
| * Verifies variants functionality for the current layout. | |||
| * Verifies variants functionality for the current layout with default implementation. | |||
There was a problem hiding this comment.
with default implementation
Quite confusing addition because of questions:
- Which default implementation?
- Implementation of what ?
Please do what's written below and change this sentence to mention the nested class.
Then it will be clear what's the default impl you mean (and please change javadoc to default impl of variant producer).
There was a problem hiding this comment.
added the link to the nested class
| @@ -39,6 +39,24 @@ | |||
| public abstract class ComponentDemoTest extends ChromeBrowserTest { | |||
| protected WebElement layout; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
| } | ||
| } | ||
|
|
||
| private Function<WebElement, String> DEFAULT_VARIANT_PRODUCER = new DefaultProducer(); |
There was a problem hiding this comment.
|
SonarQube analysis reported 4 issues
Watch the comments in this conversation to review them. 2 extra issuesNote: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
This change is