-
Notifications
You must be signed in to change notification settings - Fork 4
Added new selenium tests for administrator role #347
Conversation
def test_delete_job_without_associated_shift(self): | ||
self.login_admin() | ||
|
||
# register event first to create job | ||
event = ['event-name', '08/21/2016', '09/28/2016'] | ||
event = ['event-name', '08/21/2017', '09/28/2017'] |
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.
@smarshy Why are you changing the year? Does it help you in something?
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.
@tapasweni-pathak Yes, for event the start date has to be later than today. I had to change dates for a few functions, where the dates were set in June and then I went ahead and changed it in most places to a date later in the future so that I don't face build failures as and when the date elapses.
Later once, I put in helper functions for all files and restructure the code, these dates would have to be changed in only 1 place and I will put in logic such that the generated date is later than today
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.
@smarshy Is it the case that we need to change the dates again after we cross the current one?
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.
@tapasweni-pathak Yes, that is the case
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.
@smarshy In that case, can you please add a docstring at the top.
@smarshy Are you running pep8 after doing changes? |
@tapasweni-pathak Yes, but since all the code is to be refactored later on, I did not make any changes. Most warnings i got were related to line length, but I referred to https://www.python.org/dev/peps/pep-0008/#maximum-line-length and saw that there was flexibility offered in the length. In these tests, nearly each and every selenium command (eg. self.assertEqual(self.driver.find_element_by_xpath('//table//tbody//tr[1]//td[5]').text, 'Shifts') exceeds the 79 characters limit set. So, I increased it or else a lot of code was getting split over lots of lines. |
Refactored later on? I didn't get this. :( Related to this line length, sometimes instead of improving the readability, it decreases it. Whatever is the case, line length should be consistent all over the code. I'll see if they are different and open a issue if it different at a lot of issue. What is the line length you are sticking to right now? |
Greetings @tapasweni-pathak. I'm sticking with defaults now |
@tapasweni-pathak I was trying to stick to 90-100, but it might be a bit difficult to ensure that lines don't exceed that. I will stick to the pep8 guidelines itself and make changes in this PR |
@smarshy Okay, cool. Take a call by yourself if following pep8 line length strongly reduces the readability. Let me know if you need some examples. |
Coverage increased (+0.9%) to 94.15% when pulling 1b5198a1f4339fa3e63d1959b81a1d5d6bef6f14 on smarshy:adminTests into 7ad9ecb on systers:develop. |
@tapasweni-pathak I have modified the code as per pep8 and have added the docstring in the beginning of the file as the date has been changed in several functions |
This PR is to add the following tests for administrator roles that haven't been covered yet
Also created shift utility function so that it can be called instead of writing same code everywhere
17 new tests have been added. Total number of tests are now 150.
Related to #346