-
Notifications
You must be signed in to change notification settings - Fork 101
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
fix: work with extremely large results #304
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor things inline otherwise LGTM
} catch (Exception e) { | ||
throw new RuntimeException(e); | ||
} | ||
int sizeLimit = 15_000_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be consistent with the sizeLimit
we've set with the integrations core-npm has it set at 60_000_000
int sizeLimit = 15_000_000; | |
int sizeLimit = 60_000_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hadn't realized this was changed from the initial PR. Will adjust in all my PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
60 million is too large. Breaks webdriver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least in ruby
@@ -474,13 +472,38 @@ private Object finishRun(ArrayList<Object> partialResults) { | |||
Page blankPage = browser.newPage(); | |||
blankPage.evaluate(getAxeScript() + getAxeConfigure(hasRunPartial)); | |||
|
|||
String partialResString = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit -
This function is already quite large, breaking it out into its own function would allow commenting on what and why it was needed and can reference proposal
} catch (Exception e) { | ||
throw new RuntimeException(e); | ||
} | ||
int sizeLimit = 15_000_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be consistent with the sizeLimit
we've set with the integrations core-npm has it set at 60_000_000
int sizeLimit = 15_000_000; | |
int sizeLimit = 60_000_000; |
@@ -739,10 +744,25 @@ private Results analyzePost43x(final WebDriver webDriver, final Object rawContex | |||
|
|||
String prevWindow = WebDriverExtensions.openAboutBlank(webDriver); | |||
injectAxe(webDriver); | |||
String partialResString = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit -
This function is already quite large, breaking it out into its own function would allow commenting on what and why it was needed and can reference proposal
selenium/src/test/java/com/deque/html/axecore/selenium/Axe43xIntegrationTest.java
Show resolved
Hide resolved
This reverts commit 30fffed.
Tried to make requested changes. Unfortunately 60mil is too much for CI's memory environment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue: https://github.com/dequelabs/axe-devtools-npm/issues/1653