-
Notifications
You must be signed in to change notification settings - Fork 28
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
Refactor tests for analytics #540
Conversation
await spaceDashboardPage.analyticsCloseButton.clickWhenReady(); | ||
await dashboardPage.ready(); | ||
|
||
let codeBasesCard = await spaceDashboardPage.getCodeBaseCard(); |
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.
it's called Codebase in OSIO so the name of method should be getCodebaseCard
, the same logic for variable name
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.
OK
let analyzedCount = await analyticsCard.getAnalyzedDependenciesCount(); | ||
let unknownCount = await analyticsCard.getUnknownDependenciesCount(); | ||
|
||
expect(totalCount).toBeGreaterThanOrEqual(0, 'total dependencies count'); |
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.
Why >=0 instead of >0?
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.
Might it be the case that a value of zero for dependencies is valid? If this si true, they the code is correct as we only want to be sure to trap any invlaid (such as -1) values.
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.
What if quickstart has no dependencies? Than 0 is a perfectly valid value.
|
||
expect(totalCount).toBeGreaterThanOrEqual(0, 'total dependencies count'); | ||
expect(analyzedCount).toBeGreaterThanOrEqual(0, 'total analyzed count'); | ||
expect(unknownCount).toBeGreaterThanOrEqual(0, 'total unknown count'); |
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.
Shouldn't it be 0? Are there valid use cases when it can be non-zero?
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.
This seems correct - there might be an edge case where the value is set to zero. Is this the edge case that you are trying to handle?
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.
It was mentioned in some issue that some dependencies might be "unknown" for F8 analytics. I think we should not test the logic of the analyses, only if the results are present.
ee_tests/src/ui/base.element.ts
Outdated
if (countString === null || countString.length !== 1) { | ||
throw errorMessage + ' ' + stringNumber; | ||
} else { | ||
count = countString.map(Number)[0]; |
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.
Can't you use simply parseInt(stringNumber, 10)
?
|
||
public async getAnalyzedDependenciesCount(): Promise<number> { | ||
let text = await this.element(by.cssContainingText('b', 'Analyzed:')).getText(); | ||
let count = this.string2Number(text, 'total dependencies'); |
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.
total dependencies -> analyzed deps
|
||
public async getUnknownDependenciesCount(): Promise<number> { | ||
let text = await this.element(by.cssContainingText('b', 'Unknown:')).getText(); | ||
let count = this.string2Number(text, 'total dependencies'); |
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.
total deps -> unknown deps
Merge is blocked by openshiftio/openshift.io#2729 - I cannot verify that it runs correctly |
Merged |
This should be prerequisite for merging analytics tests to deployments tests.