-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
331a013
to
23c179e
Compare
5a71384
to
8775749
Compare
vms/job/tests/test_jobDetails.py
Outdated
['event', '2017-06-15', '2017-06-17']) | ||
created_job = create_job_with_details( | ||
['job', '2017-06-15', '2017-06-18', '', created_event]) | ||
pass |
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.
Try logging out for every test otherwise logout 1 time in class tear down.
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.
@naman1901 @pavankm I have a doubt regarding this.
If we logout only once i.e. in tearDownClass
then build time won't be affected but if we logout each time i.e. in tearDown
the build time will increase since it will logout for each test.
On the other hand the if we logout once that is less correct than each time, since each test is considered independent of the other and should end their session at end of test.
Need your views on which approach to go with.
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.
Do you have empirical data on how big a difference it makes?
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.
Umm not really but I did notice ~5 minutes minimum increase in boot time although the bu8ld time doesn't really increase by more than ~1 minute
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.
Ideally, we should follow the correct approach even if it takes more time. Waiting for @pavankm to present his views.
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.
Log out every session
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.
I also think the same.
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.
Need some changes.
job_details_page.click_link(job_details_page.jobs_tab) | ||
self.assertEqual(job_details_page.remove_i18n(self.driver.current_url), | ||
self.live_server_url + job_details_page.job_list_page) | ||
self.assertEqual(job_details_page.get_message_context(), |
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.
Make this a variable inside JobDetailsPage
itself (multiple uses might be possible.)
vms/job/tests/test_jobDetails.py
Outdated
job_details_page.click_link(job_details_page.create_job_tab) | ||
self.assertEqual(job_details_page.remove_i18n(self.driver.current_url), | ||
self.live_server_url + job_details_page.create_job_page) | ||
self.assertEqual(job_details_page.get_message_context(), 'Please add events to associate with jobs first.') |
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.
Same as above.
vms/job/tests/test_jobDetails.py
Outdated
job_details_page.fill_job_form(job_start_after_end) | ||
|
||
# Check error. | ||
self.assertEqual(job_details_page.get_job_start_date_error(), 'Start date must be before the end date') |
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.
Same.
vms/job/tests/test_model.py
Outdated
self.assertEqual(str(job_in_db.end_date), job[2]) | ||
self.assertEqual(job_in_db.description, job[3]) | ||
|
||
# Checks are missing from model class |
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.
Raise issue after confirmation in tomorrow's meeting.
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.
Workaround found:
model validation checks aren't performed until a full_clean
is called explicitly from code/command-line, Forms perform full_clean
on submit thats why the error messages are shown. Thus model.save()
will save the instance unless there not-null constraint is broken.
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 is the confusion here?
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.
Its resolved, by mistake I wrote workaround found instead should be Solution found
vms/pom/pages/jobDetailsPage.py
Outdated
def get_message_context(self): | ||
return self.events_page.get_message_context() | ||
|
||
@staticmethod |
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.
Method can be part of BasePage
will be needed by all pages.
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.
Looks good. I'll approve once there is clarity on the multiple login issue
Review awaited from Anjali, Update and finalise PR and ping me on Slack @Monal5031 |
a718e68
to
e55486d
Compare
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.
LGTM
vms/job/tests/test_jobDetails.py
Outdated
['event', '2017-06-15', '2017-06-17']) | ||
created_job = create_job_with_details( | ||
['job', '2017-06-15', '2017-06-18', '', created_event]) | ||
pass |
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.
I also think the same.
Description
Tested Job app
Fixes #686
Type of Change:
Delete irrelevant options.
Code/Quality Assurance Only
How Has This Been Tested?
To verify that tests are non-breaking change in job app use:
and to overall project use:
Checklist:
Code/Quality Assurance Only