forked from tvarchive/codingRound
-
Notifications
You must be signed in to change notification settings - Fork 0
/
Copy pathcodeReview.txt
63 lines (43 loc) · 2.26 KB
/
codeReview.txt
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
## Code review
* Liner frameworks is easy to understand, need to updated it other framework.
* try "not" to hard code the values inside script file. should make it to read from excel or some data file.
( For eg: "https://www.cleartrip.com/", "Delhi")
* Could have added functionality to handle Multiple browser.
* Could have moved reusable methods like "setDriverPath" to common file, so that we can have less maintainability.
**FlightBookingTest.java**
- waitFor(2000);
* using static timeout would lead to flaky test, we should use methods 'waitForElement' instead
- driver.findElement(By.id("OneWay")).click();
* Selenium Actions could be moved to separate files, for maintainability,
* Locators could be moved to separate files(POM), where elements are reused and maintained easily
- driver.findElement(By.xpath("//*[@id='ui-datepicker-div']/div[1]/table/tbody/tr[3]/td[7]/a")).click();
* can avoid index, Since they have greater probability of change, during enhancements
- Assert.assertTrue(isElementPresent(By.className("searchSummary")));
* can have some comment messages on failures, which will make others to understand the failures. also help reports more readable
- private void waitFor(int durationInMilliSeconds) {
try {
Thread.sleep(durationInMilliSeconds);
} catch (InterruptedException e) {
e.printStackTrace(); //To change body of catch statement use File | Settings | File Templates.
}
}
* could have designed this method to wait for element, instead static wait shoudl move to some resuable folder
- private boolean isElementPresent(By by) {
try {
driver.findElement(by);
return true;
} catch (NoSuchElementException e) {
return false;
}
}
* Though this method Looks good, could have added default parameter to handle wait also raise
**HotelBookingTest.java**
- WebDriver driver = new ChromeDriver();
* should initialise in common component
- localityTextBox.sendKeys("Indiranagar, Bangalore");
could have read values from data sheets, or other mode
**SignInTest.java**
- driver.quit();
on the test files will kill the driver, which will drivers unavailable for other tests.
- WebDriver driver = new ChromeDriver();
Should be in @Beforeall Annotation