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

Bugfix/accessibility duplicates #439

Merged
merged 23 commits into from
Jan 23, 2019

Conversation

wblachowski
Copy link
Contributor

@wblachowski wblachowski commented Nov 23, 2018

Fix line and column numbers duplicating in reports when multiple elements have the same HTML. (related to #438)

Description

My implementation scans document HTML starting from the latest occurrence of the same issue and not from the beginning of the document.


I hereby agree to the terms of the AET Contributor License Agreement.

Copy link
Contributor

@malaskowski malaskowski left a comment

Choose a reason for hiding this comment

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

I like this fix. Could you please provide some unit tests for AccessibilityCollector logic?

@wblachowski
Copy link
Contributor Author

Separated position finding logic from the rest of the collector.
Provided some unit tests for issue position finder.

Copy link
Contributor

@malaskowski malaskowski left a comment

Choose a reason for hiding this comment

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

Thanks for the unit tests. Last - cosmetic comments below.

@@ -64,7 +63,8 @@ public CollectorStepResult collect() throws ProcessingException {
.getExecutionResultAsString();
final String json = jsExecutor.execute(script, standard).getExecutionResultAsString();
List<AccessibilityIssue> issues = parseIssues(json);
getElementsPositions(issues, html);
AccessibilityIssueMarkupFinder issuesFinder = new AccessibilityIssueMarkupFinder(html,issues);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about moving parseIssues(json) logic to AccessibilityIssueMarkupFinder.

Then code here would look like:

  List<AccessibilityIssue> issuesWithPositions = new AccessibilityIssueMarkupFinder(html, json).execute(); // or .find()/.get()/etc.

}

@Test
public void testSingleIssueNoOffset() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please adjust unit test naming convention to the AET tests-naming-convention?

Copy link
Contributor

@tkaik tkaik left a comment

Choose a reason for hiding this comment

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

Please update line/column numbers in accessibility filters configured in integration-tests/test-suite/partials/accessibility-filtered.xml - probably after introdcuing your fix, the line/column location of errors will be different. Please test it on your branch by running the whole test-suite - you can find instructions in integration-tests/README.md

@@ -6,6 +6,9 @@ necessary configuration changes that were introduced in comparison to previously
You may see all changes in the [Changelog](https://github.com/Cognifide/aet/blob/master/CHANGELOG.md).

## Unreleased
Changes:
* Counting line and column number of an accessibility issue occurrence in [[Accessibility Collector|AccessibilityCollector]] has been improved.
Columns are now indexed starting from 1. Bug with two identical issues on a single page yielding the same line and column number has been fixed.
Copy link
Contributor

@tkaik tkaik Dec 3, 2018

Choose a reason for hiding this comment

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

  • Please provide some links here to related github issue(s) :)
  • Pleasy explicitly explain here how users should change their suite.xml when upgrading from previous AET version - e.g. "When upgrading from previous AET version, you may need to decrease column value in by 1 and change line value to match (...)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@malaskowski malaskowski added the QA Required Requires manual tests, possible regression or impact on existing features. label Dec 11, 2018
@Asia95
Copy link

Asia95 commented Jan 22, 2019

tested locally, working well ✔️

Copy link
Contributor

@tkaik tkaik left a comment

Choose a reason for hiding this comment

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

Please move changelog entry to Unreleased section

@tkaik tkaik merged commit e463084 into wttech:master Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA Required Requires manual tests, possible regression or impact on existing features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants