-
Notifications
You must be signed in to change notification settings - Fork 172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add a JS unit test to verify filterSubVerticals #4561
base: master
Are you sure you want to change the base?
Conversation
87dd832
to
e4f966c
Compare
71d27e9
to
0249489
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.
Just a couple of comments; otherwise, LGTM
def test_filter_sub_verticals_javascript(self): | ||
""" | ||
Verify that selecting a vertical with one sub-vertical (AI and Business) shows the expected single sub-vertical | ||
and a vertical with two sub-verticals ('Data') shows both sub-verticals. |
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.
not sure if we need to add vertical and subvertical name in docstring
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.
Removing it.
self.vertical = VerticalFactory(name='AI') | ||
self.sub_vertical = SubVerticalFactory(name='Machine Learning', vertical=self.vertical) | ||
|
||
self.other_vertical = VerticalFactory(name='Business') |
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.
we can use some dummy test verticals instead of adding actual verticals here
) | ||
return self.get_visible_options('sub_vertical') | ||
|
||
def test_filter_sub_verticals_javascript(self): |
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.
btw this testcase is failing
a3d2d8b
to
5a11233
Compare
super().setUp() | ||
ContentType.objects.clear_cache() | ||
|
||
self.user = UserFactory(username='superuser_func', is_superuser=True, is_staff=True) |
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.
Is superuser
a better username?
self.driver.get(self.url) | ||
|
||
def _login(self): | ||
"""Log into Django via test client, then add the session cookie to Selenium.""" |
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.
Would it be better to log in naturally? i.e through the browser?
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.
ProgramAdminFunctionalTests does it like that.
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.
That's what I did earlier ;-;
super().setUpClass() | ||
firefox_options = Options() | ||
firefox_options.headless = True | ||
cls.driver = webdriver.Firefox(options=firefox_options) |
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.
[nit] cls.browser?
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.
Isn't it the same?
"document.getElementById('vertical').dispatchEvent(new Event('change'));" | ||
) | ||
# Wait until the visible sub-vertical options match the expected set. | ||
WebDriverWait(self.driver, 5).until( |
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.
We have an implicit_wait
in the init too. Are both these waits needed?
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.
Both waits are not particularly “redundant” because implicitly_wait applies whenever we have a find_element()
but this explicit wait is to apply the change event on the element.
self.assertEqual(len(visible_options), 2) | ||
self.assertEqual(option_values, expected_values) | ||
|
||
|
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.
We should have a test for the initial load too i.e before the user has interacted with the page. I recall that being a discussed during develppment
5a11233
to
27ad269
Compare
PROD-4296
Adds selenium-based unit tests to verify JS script added to load subverticals on the course tagging detail page.