You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/05/13 17:13:01 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5845: Migrate cache group test from E2E to integration test
ocket8888 commented on a change in pull request #5845:
URL: https://github.com/apache/trafficcontrol/pull/5845#discussion_r631946121
##########
File path: traffic_portal/test/integration/PageObjects/CacheGroup.po.ts
##########
@@ -117,16 +116,15 @@ export class CacheGroupPage extends BasePage {
if (cachegroup.Type == "EDGE_LOC") {
const name = cachegroup.FailoverCG + this.randomize;
await this.txtFailoverCG.click();
- if(await browser.isElementPresent(element(by.xpath(`//select[@name="fallbackOptions"]//option[@label="`+ name + `"]`)))){
- await element(by.xpath(`//select[@name="fallbackOptions"]//option[@label="`+ name + `"]`)).click();
- }else{
+ if (await browser.isElementPresent(element(by.xpath(`//select[@name="fallbackOptions"]//option[@label="` + name + `"]`)))) {
+ await element(by.xpath(`//select[@name="fallbackOptions"]//option[@label="` + name + `"]`)).click();
+ } else {
result = undefined;
}
}
await this.txtType.sendKeys(cachegroup.Type);
await snp.ClickUpdate();
- if(result != undefined)
- {
+ if (result != undefined) {
Review comment:
comparison should use `===`/`!==`
##########
File path: traffic_portal/test/integration/PageObjects/CacheGroup.po.ts
##########
@@ -117,16 +116,15 @@ export class CacheGroupPage extends BasePage {
if (cachegroup.Type == "EDGE_LOC") {
const name = cachegroup.FailoverCG + this.randomize;
await this.txtFailoverCG.click();
- if(await browser.isElementPresent(element(by.xpath(`//select[@name="fallbackOptions"]//option[@label="`+ name + `"]`)))){
- await element(by.xpath(`//select[@name="fallbackOptions"]//option[@label="`+ name + `"]`)).click();
- }else{
+ if (await browser.isElementPresent(element(by.xpath(`//select[@name="fallbackOptions"]//option[@label="` + name + `"]`)))) {
Review comment:
nit/protip: since you're already using a template string, you can just interpolate `name` instead of concatenating:
```typescript
`//select[@name="fallbackOptions"]//option[@label="${name}"]`
```
##########
File path: traffic_portal/test/integration/PageObjects/CacheGroup.po.ts
##########
@@ -158,6 +156,31 @@ export class CacheGroupPage extends BasePage {
await snp.NavigateToCacheGroupsPage();
return result;
}
+ public async CheckCSV(name: string): Promise<boolean> {
+ let linkName = name;
+ let result = false;
+ if (await browser.isElementPresent(element(by.xpath("//span[text()='" + linkName + "']"))) == true) {
Review comment:
You can get links by their text using `by.linkText` or `by.partialLinkText`. However, despite the name of that variable, it looks like it's not actually a link? If this is actually just a `SPAN` element with a text node child containing the name, you could do `by.cssContainingText("span", linkName)` to get elements matching the CSS selector `span` with a text node containing `linkName`.
I just find XPath selectors harder to read than css or element selectors.
##########
File path: traffic_portal/test/integration/PageObjects/CacheGroup.po.ts
##########
@@ -158,6 +156,31 @@ export class CacheGroupPage extends BasePage {
await snp.NavigateToCacheGroupsPage();
return result;
}
+ public async CheckCSV(name: string): Promise<boolean> {
+ let linkName = name;
+ let result = false;
+ if (await browser.isElementPresent(element(by.xpath("//span[text()='" + linkName + "']"))) == true) {
+ result = true;
+ }
+ return result;
+ }
Review comment:
It looks like `linkName` is only used as an alias of the `name` argument, which is unnecessary because strings are immutable (and that assignment would be a shallow copy of an implicit reference even if they weren't - and also the value is only read, not manipulated). Likewise, `result` is only used to contain the result of a single operation that is immediately returned. Finally, `browser.isElementPresent` returns a `Promise<boolean>` (although the API weakly types its return value to `webdriver.promise.Promise<any>` for some reason).
With all this in mind, I think this method can actually just be a single line:
```typescript
public async CheckCSV(name: string): Promise<boolean> {
return browser.isElementPresent(element(by.cssContainingText("span", name)));
}
```
Or, for just a little more type-safety, you actually don't need to use `browser` at all, [since an element can check for its own presence](https://www.protractortest.org/#/api?view=ElementFinder.prototype.isPresent):
```typescript
public async CheckCSV(name: string): Promise<boolean> {
return element(by.cssContainingText("span", name)).isPresent();
}
```
##########
File path: traffic_portal/test/integration/PageObjects/CacheGroup.po.ts
##########
@@ -117,16 +116,15 @@ export class CacheGroupPage extends BasePage {
if (cachegroup.Type == "EDGE_LOC") {
const name = cachegroup.FailoverCG + this.randomize;
await this.txtFailoverCG.click();
- if(await browser.isElementPresent(element(by.xpath(`//select[@name="fallbackOptions"]//option[@label="`+ name + `"]`)))){
- await element(by.xpath(`//select[@name="fallbackOptions"]//option[@label="`+ name + `"]`)).click();
- }else{
+ if (await browser.isElementPresent(element(by.xpath(`//select[@name="fallbackOptions"]//option[@label="` + name + `"]`)))) {
Review comment:
Also, I wouldn't ask you to change it in this PR, but I think most people are more familiar with CSS selectors than XPath, so this could be written as:
```typescript
by.css(`select[name="fallbackOptions"] > option[label="${name}"]`)
```
##########
File path: traffic_portal/test/integration/specs/CacheGroup.spec.ts
##########
@@ -41,20 +41,38 @@ cachegroups.tests.forEach(cacheGroupData => {
await cacheGroupPage.OpenTopologyMenu();
await cacheGroupPage.OpenCacheGroupsPage();
})
+ cacheGroupData.check.forEach(check => {
+ it(check.description, async () => {
+ expect(await cacheGroupPage.CheckCSV(check.Name)).toBeTruthy();
Review comment:
`CheckCSV` returns a `Promise<boolean>`, so this can check specifically for `.toBeTrue()`, not merely `.toBeTruthy()`. Or you could use `.toBe(true)`, the difference between that and `.toBeTrue()` is convoluted and involves using `Boolean` as an ES6 constructor - which you should never do anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org