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

Investigate problem triggering assertion in Alignment#createAlignment() #598

Closed
3 of 8 tasks
githubert opened this issue Jan 13, 2020 · 6 comments · Fixed by #788
Closed
3 of 8 tasks

Investigate problem triggering assertion in Alignment#createAlignment() #598

githubert opened this issue Jan 13, 2020 · 6 comments · Fixed by #788
Assignees
Labels
bug Something isn't working

Comments

@githubert
Copy link

githubert commented Jan 13, 2020

Before submission, please check that ...

  • this is related specifically to recheck and not its extension (e.g. recheck-web).
  • the documentation does not mention a fix (e.g. wrong API usage, ...).
  • the bug is not related to a customization (e.g. custom implementations).
  • there are no open or closed issues that are related to this problem.

Describe the Bug

When running the integration tests for recheck-web, an assertion in recheck is triggered. (The problem is visible in the recheck-web integration tests, but stems from a problem in recheck)

[ERROR]   RecheckRemoteWebElementIT.complete_html_should_be_same_as_driver:38 bestMatch and previousMatch have a match of 100%? At least paths should differ! html[1]/body[1]/form[3] # br # 1 == html[1]/body[1]/form[3] # br # 1
[ERROR]   RecheckRemoteWebElementIT.complete_html_should_be_same_as_web_element:49 bestMatch and previousMatch have a match of 100%? At least paths should differ! html[1]/body[1]/form[3] # br # 1 == html[1]/body[1]/form[3] # br # 1

This has not been present on earlier runs on the CI system when using Ubuntu 14.04.

How to Reproduce?

Check out retest-web, run mvn clean verify on Fedora 31 or Ubuntu 18.04.

Setup

  • recheck: 1.8.0
  • recheck-web: 8ef03a8eb49a136623f0d4608284e1eb28806e7a
  • mvn 3.6.1 (Fedora 31),
  • OS: Fedora 31 / Ubuntu 18.04 (Bionic Beaver)
  • Java version: Fedora 31: 13.0.1, Ubuntu: 11.0.2 (mvn compile) & 13.0.1 (rest of the steps)

Steps to resolving this issue

This involves work across both projects.

  • Reduce form-page.html to the minimal HTML needed to reproduce this issue (recheck-web)
  • Create a test that can reproduce this issue (recheck)
  • Fix underlying problem such that the test will not fail anymore (recheck)
  • Removed @Disabled from the failing tests in RecheckRemoteWebElementIT (recheck-web )
@githubert githubert added the bug Something isn't working label Jan 13, 2020
@githubert githubert self-assigned this Jan 13, 2020
@githubert
Copy link
Author

After further investigation it turns out that this stems from the following issue:

The review tool added two duplicate entries to a retest.xml file after accepting the changes, which caused the tests to fail. A consistency check may have drawn the attention to these duplicate entries earlier.

In order to reproduce this issue:

  1. Clone recheck-web
  2. Run git checkout a427151328ae7766e0426de4710eea54e5f21de5
  3. Run mvn -Dwebdriver.gecko.driver=/path/to/bin/geckodriver -Dde.retest.recheck.rehub.reportUploadEnabled=false -Dit.test=RecheckRemoteWebElementIT verify
  4. Note that the tests fail, a report file is generated
  5. Download the review tool from https://assets.retest.org/releases/review.html (tested with version 1.8.0)
  6. Call review target/test-classes/retest/recheck/de.retest.web.it.RecheckRemoteWebElementIT.report
  7. Accept the two changes involving <br>
  8. Apply the changes
  9. git diff should report added information for br-4 and br-5, but everything is repeated twice

@githubert
Copy link
Author

This also happens using the retest.cli tool, as expected.

Run recheck commit --all target/test-classes/retest/recheck/de.retest.web.it.RecheckRemoteWebElementIT.report instead of using the review tool.

@beatngu13
Copy link
Contributor

Good catch! AFAIK both recheck.cli and review use ApplyChangesToStatesFlow from recheck, so I assume the error must be somewhere below there …

@githubert
Copy link
Author

I noticed that in the UI the elements are already shown twice, which I found odd. Upon further inspection it turns out that the two affected tests in recheck-web use the same name for @ParameterizedTest.

See: https://github.com/retest/recheck-web/blob/530d7ffd97ef1dc1f34f839cabaa0fc53203c782/src/test/java/de/retest/web/it/RecheckRemoteWebElementIT.java#L30-L50

Changing the names, adding folders for the new names and copying the existing retest.xml files to the new folders (two for each test), will yield the same failure on the first run, but accepting the changes will then result in the tests succeeding.

I feel a bit bad that I opened the issue here and not in recheck-web ;-).

@diba1013
Copy link
Contributor

diba1013 commented Aug 29, 2020

Since I have stumbled upon this again (previously avoided via recreating the GM), I will try to take a look into this in more depth, as I currently have a reproducible test case.

  • I will try to create a minimal test to reproduce this within recheck.
  • Try to come up with a fix, as mentioned above, I think that this is somewhere in Element#applyChanges.
  • Look if these fixes also work in recheck-web.

@diba1013 diba1013 assigned diba1013 and unassigned githubert Aug 29, 2020
@diba1013
Copy link
Contributor

A short investigation into the failing tests in recheck-web shows, that it seems to be related to the volatile (absolute-)outline, which makes the IdentifyingAttributes#equals method essentially useless for testing equality between multiple runs. This method is used to determine, if the element should be removed from the parent upon apply.

While this is certainly no solution, but more a workaround, I would propose the same steps done with #726: Use the IdentifyingAttributes#identifier method to determine equality.

Because this has been the cause for multiple issues, we should give retest/recheck-web#200 a rethink as a mid-term solution. But long term, we should rethink this approach and how we handle such volatile attributes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

3 participants