Administrator Tests #695
Administrator Tests #695
Conversation
Monal5031
added
Difficulty: MEDIUM
Program: GSOC
|
Debug Testing here: test |
|
Coverage increased (+28.8%) to 71.261% when pulling 6539403ee571a5164f3424da4ab21d26db167f67 on Monal5031:testing-admin into cf79a03 on systers:gsoc18-infra. |
| @@ -75,6 +75,27 @@ def create_volunteer_with_details(volunteer): | |||
| return v1 | |||
|
|
|||
|
|
|||
| def create_volunteer_with_details_dynamic_password(volunteer): | |||
| """ | |||
There was a problem hiding this comment.
please no. What is the use case of passing unrelated data in a list ? Please use a proper dict :-(
There was a problem hiding this comment.
I was thinking for the same, (I hate using lists).
but the above related functions were using list so I didn't want to change the similarities.
There was a problem hiding this comment.
Also @tonythomas01 can you please help me solve those broken pipe issue in travis without increasing the build time (it already 7 mins and I've added only 1 app).
There was a problem hiding this comment.
okey - in that case, can you create a different task for changing this abuse of lists to dicts ? Let me look at the build steps as well.
There was a problem hiding this comment.
Okay I looked on the dependency of this change. Its quite huge should we really change it? There are other instances too where we should've used dict like fill_form functions.
There was a problem hiding this comment.
I am not sure about other instances, but if a function receives data about an instance and populates the instance model to create a new one - that is a dictionary use case. It would be huge depending on how much things are here, but hopefully somebody else would take care of it I guess.
There was a problem hiding this comment.
Sure then I'll create an issue for it. 👍 .
Also should the form filling in e2e tests is a dict use case right? (I mean when we pass info to that fill function) We use send_keys() to send the info at the end.
There was a problem hiding this comment.
I think yes. See, when its structured data - yes dict. Collection of similar stuff - list. Even though, there is nothing wrong in having mixed data in a list though, like say awkward_list = [1, 2, 'myvar', {'a_json_key': 'a_json_value'}, 345.4]
There was a problem hiding this comment.
Thanks. that makes things quite clear.
| @@ -39,7 +34,7 @@ def setUpClass(cls): | |||
| def setUp(self): | |||
| create_admin() | |||
| self.login_admin() | |||
| self.settings.go_to_events_page() | |||
| self.driver.implicitly_wait(50) | |||
There was a problem hiding this comment.
50 seconds ? really ? why do you want this ? please no.
| @@ -52,21 +47,29 @@ def tearDownClass(cls): | |||
| def check_event_form_values(self, event): | |||
| settings = self.settings | |||
| self.assertEqual(settings.get_event_name_value(), event[0]) | |||
| self.driver.implicitly_wait(10) | |||
| self.assertEqual(settings.get_shift_end_time_value(), shift[2]) | ||
| self.driver.implicitly_wait(10) | ||
| self.assertEqual(settings.get_shift_max_volunteers(), shift[3]) |
There was a problem hiding this comment.
remove all sleep times please. if you want to have a sleep time - that too > 2 probably there is something wrong.
There was a problem hiding this comment.
@tonythomas01 Check these builds build1 and build2 Removing the implicit waits cause 33 of them to fail (locally 8 are failing).
I agree there are a lot of implicit waits and which is not a good practice to use but the redirects are fast that the forms are not being filled within the time frame and elements to be selected are refreshed.
All these tests work individually but as a whole they get a wait problem. 😕
Any suggestions how I can tackle this?
There was a problem hiding this comment.
Nevermind. Made some tweaks in my changes and it worked on my test branch.
Waiting for build on this PR.
|
|
||
| # database check to ensure that job not created | ||
| # Check Database of job creation | ||
| self.assertEqual(len(Job.objects.all()), 0) |
There was a problem hiding this comment.
to be honest, I see a bigger problem - if your tests are doing E2E stuff - then you should nto even care about what the database looks like here. but, I think this is not in scope of this PR.
There was a problem hiding this comment.
Shouldn't we check for loopholes in backend if invalid instance are being created in db even after giving some error message in front?
There was a problem hiding this comment.
It's an antipattern for an E2E test. For a unit test, yes you should. In E2E - you look at direct user interactions. Cannot quote anything as from phone, but feel free to look it up.
|
@tonythomas01 Any more changes needed? otherwise please mark approve. |
| [ | ||
| { | ||
| "volunteer": [ | ||
| "uname1", |
There was a problem hiding this comment.
this representation is a start, but only half done. You need it like this.
"volunteer": {
"first_name": "uname1",
"last_name": "uname1",
....
}
and then use it as:
v1 = Volunteer(
+ email=volunteer['email'],
+ first_name=volunteer['first_name'],
+ last_name=volunteer['last_name'],
....
+ )
There was a problem hiding this comment.
Umm... if I change this then I'd have to change the structure we earlier discussed (lists -> dict), I think this can be a part of the new issue we'll create to change use of lists to dict.
There was a problem hiding this comment.
okey then. Once you link that new issue here, I will finish my review :)
There was a problem hiding this comment.
here you go @tonythomas01 #697
There was a problem hiding this comment.
Looks good then. approving.
There was a problem hiding this comment.
I agree with @tonythomas01 . Will this issue be taken up by you, @Monal5031 ?
There was a problem hiding this comment.
Not atm, but if time persists before I move on to postorius I'll do it.
| """ | ||
| u1 = User.objects.create_user(username=volunteer[0], password=volunteer[1]) | ||
| v1 = Volunteer( | ||
| email=volunteer[2], |
There was a problem hiding this comment.
this and above will change after fixing my comment above.
| Creates and returns volunteer with passed name and dates | ||
| """ | ||
| u1 = User.objects.create_user(username=volunteer[0], password=volunteer[1]) | ||
| v1 = Volunteer( |
There was a problem hiding this comment.
inconsistency (minor though). Why not use Volunteer.objects.create() as you have used above (I know you have used a diff manager function, but the second one is a model instantiation via constructor which is different.
There was a problem hiding this comment.
The similar functions above have used this so I went with that.
There was a problem hiding this comment.
also, some general notes - please avoid using these kind of variables. u1, a1, b2, c3'. it should be user_1or even betteruser_a, user_b`, etc.
Description
Added tests for administrator app
Fixes #685
Type of Change:
Delete irrelevant options.
Code/Quality Assurance Only
How Has This Been Tested?
To test my tests:
To confirm non-breaking change:
Checklist:
Delete irrelevant options.
Code/Quality Assurance Only