Update E2ETest requirements #11634
Update E2ETest requirements #11634
Conversation
- Do not enforce prerequisites by default - Warn if prerequistes are not met unless explicitly enforced - Run yarn install by default if Java is available
| <SeleniumE2ETestsSupported Condition="'$(SeleniumE2ETestsSupported)' == '' and '$(TargetArchitecture)' != 'arm' and '$(OS)' == 'Windows_NT' and '$(BuildJava)' == 'true'">true</SeleniumE2ETestsSupported> | ||
| <EnforcePrerequisites Condition="'$(SeleniumE2ETestsSupported)' == 'true' and '$(EnforcePrerequisites)' == ''">true</EnforcePrerequisites> | ||
| <SeleniumE2ETestsSupported Condition="'$(SeleniumE2ETestsSupported)' == '' and '$(TargetArchitecture)' != 'arm' and '$(OS)' == 'Windows_NT'">true</SeleniumE2ETestsSupported> | ||
| <EnforcePrerequisites Condition="'$(SeleniumE2ETestsSupported)' == 'true' and '$(EnforcePrerequisites)' == ''">false</EnforcePrerequisites> |
There was a problem hiding this comment.
I'm of the opinion that we can remove this property and just go with the default behaviour. We'll never enforce prereqs and you will need to install java for tests to work. The difference is that instead of errors at build time, you'll see errors during test instead.
There was a problem hiding this comment.
Then why aren't you removing the property?
But, the current $(EnforcePrerequisites) default is correct and keeping the property allows users to disable the checks once they have everything set up.
| Condition="'$(ErrorCode)' == '0'" | ||
| Importance="High" | ||
| Text="JAVA is available on the PATH" /> | ||
| <Message |
There was a problem hiding this comment.
Could consider making this a warning instead, applies in a few places.
There was a problem hiding this comment.
As I said above, get rid of the '$(EnforcePrerequisites)' == 'false' cases entirely. If a user passes /p:EnforcePrerequisites=false on the command line, assume they know Java is where it should be.
| <SeleniumE2ETestsSupported Condition="'$(SeleniumE2ETestsSupported)' == '' and '$(TargetArchitecture)' != 'arm' and '$(OS)' == 'Windows_NT' and '$(BuildJava)' == 'true'">true</SeleniumE2ETestsSupported> | ||
| <EnforcePrerequisites Condition="'$(SeleniumE2ETestsSupported)' == 'true' and '$(EnforcePrerequisites)' == ''">true</EnforcePrerequisites> | ||
| <SeleniumE2ETestsSupported Condition="'$(SeleniumE2ETestsSupported)' == '' and '$(TargetArchitecture)' != 'arm' and '$(OS)' == 'Windows_NT'">true</SeleniumE2ETestsSupported> | ||
| <EnforcePrerequisites Condition="'$(SeleniumE2ETestsSupported)' == 'true' and '$(EnforcePrerequisites)' == ''">false</EnforcePrerequisites> |
There was a problem hiding this comment.
Then why aren't you removing the property?
But, the current $(EnforcePrerequisites) default is correct and keeping the property allows users to disable the checks once they have everything set up.
| @@ -23,19 +19,46 @@ | |||
|
|
|||
| <!-- Running prerequisites --> | |||
|
|
|||
| <Target Name="EnsurePrerequisites" Condition="'$(EnforcePrerequisites)' == 'true'" BeforeTargets="EnsureNodeJSRestored"> | |||
| <Target Name="EnsurePrerequisites"> | |||
There was a problem hiding this comment.
Much preferred the previous approach of only executing this target when $(EnforcePrerequisites) is enabled. As-is, the property only slightly changes the messages but doesn't make a real difference. There's a message already about $(EnforcePrerequsites) being disabled in EnsureNodeJSRestored. And, doubling all the messages makes the target harder to read.
| Condition="'$(ErrorCode)' == '0'" | ||
| Importance="High" | ||
| Text="JAVA is available on the PATH" /> | ||
| <Message |
There was a problem hiding this comment.
As I said above, get rid of the '$(EnforcePrerequisites)' == 'false' cases entirely. If a user passes /p:EnforcePrerequisites=false on the command line, assume they know Java is where it should be.
analogrelay
added
the
area-infrastructure
|
I wasn't sure how to explain my thoughts properly, so I made a PR. I think this would be easier, wouldn't it? #11642 It makes sure the prerequisites are enforced when building from VS or on the CI in supported platforms and enforces them when building from the command-line for projects that have E2E tests. Shouldn't that be enough? |
Perhaps. This PR does have a couple of advantages over #11642:
|
|
This PR has a way bigger scope than the one I sent out.
Mine does too.
How is this an advantage? I would see it as a disadvantage. It only adds time and potential flakiness
Don't think that is a problem at all. Specially given that it literally does it in two places.
I don't even know how to answer this because its not even clear what you are suggesting, but perceive this comment largely as a personal opinion, and I'm not sure I agree with it. Alternatively the changes I suggested:
I'm open to revisit how things work in this area in the future, but right now the priority is to make the least risky think that makes things work/run again as this blocks any candidate build. Hope this helps. |
|
Sure go for it @javiercn |
Updating the default behaviour.
Addresses #11342.