-
-
Notifications
You must be signed in to change notification settings - Fork 221
Improved testing 2 #1150
Improved testing 2 #1150
Changes from all commits
734d33f
1a63f4b
6993e64
d4e112a
646d361
0648850
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| # *************************************** | ||
| # |docname| - pytest fixtures for testing | ||
| # *************************************** | ||
| # This defines fixtures specific to the client for test. These same fixtures are defined differently on the server to accommodate the different setup these. | ||
| # | ||
| # TODO: rewrite the tests to use pytest-native syntax, then put the contents of ``unittest_base`` here. | ||
| # | ||
| # Imports | ||
| # ======= | ||
| # These are listed in the order prescribed by `PEP 8 | ||
| # <http://www.python.org/dev/peps/pep-0008/#imports>`_. | ||
| # | ||
| # Standard library | ||
| # ---------------- | ||
| # None. | ||
| # | ||
| # Third-party imports | ||
| # ------------------- | ||
| import pytest | ||
|
|
||
|
|
||
| # Local imports | ||
| # ------------- | ||
| # This is necessary to bring in the shared pytest fixture. | ||
| from runestone.shared_conftest import _SeleniumUtils, selenium_driver # noqa: F401 | ||
| from runestone.unittest_base import ModuleFixture, HOST_URL, run_webpack # noqa: F401 | ||
|
|
||
|
|
||
| # Fixtures | ||
| # ======== | ||
| @pytest.fixture(scope="module") | ||
| def selenium_driver_session(request): | ||
| mf = ModuleFixture(request.fspath, True) | ||
| mf.setUpModule() | ||
| yield mf.driver | ||
| mf.tearDownModule() | ||
|
|
||
|
|
||
| # Present ``_SeleniumUser`` as a fixture. To use, provide it with a ``_TestUser`` instance. | ||
| @pytest.fixture | ||
| def selenium_utils(selenium_driver): # noqa: F811 | ||
| return _SeleniumUtils(selenium_driver, HOST_URL) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,8 @@ export default class Poll extends RunestoneBase { | |
| this.getQuestionText(); | ||
| this.getOptionText(); //populates optionList | ||
| this.renderPoll(); //generates HTML | ||
| this.checkPollStorage(); //checks localStorage to see if this poll has already been completed by this user | ||
| // Checks localStorage to see if this poll has already been completed by this user. | ||
| this.checkPollStorage(); | ||
| this.caption = "Poll"; | ||
| this.addCaption("runestone"); | ||
| } | ||
|
|
@@ -59,7 +60,7 @@ export default class Poll extends RunestoneBase { | |
| this.containerDiv = document.createElement("div"); | ||
| this.pollForm = document.createElement("form"); | ||
| this.resultsDiv = document.createElement("div"); | ||
| this.containerDiv.id = this.divid + "_container"; | ||
| this.containerDiv.id = this.divid; | ||
|
bjones1 marked this conversation as resolved.
|
||
| $(this.containerDiv).addClass(this.origElem.getAttribute("class")); | ||
| $(this.pollForm).html( | ||
| `<span style='font-size: Large'>${this.question}</span>` | ||
|
|
@@ -212,6 +213,7 @@ export default class Poll extends RunestoneBase { | |
| } | ||
| $(this.resultsDiv).append(list); | ||
| } | ||
| this.indicate_component_ready(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If its called in the base class why call it again here?? Shouldn't there be one and only one indication of when the component is ready. Otherwise we may get multiple firings of things that should not be done multiple times.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's only called in the base class if Here's the flow of this component, as I understand it:
...which is one of the more complex flows I've seen in the components. |
||
| } | ||
| disableOptions() {} | ||
| checkPollStorage() { | ||
|
|
@@ -227,7 +229,9 @@ export default class Poll extends RunestoneBase { | |
| eBookConfig.ajaxURL + "getpollresults", | ||
| data, | ||
| this.showPollResults.bind(this) | ||
| ); | ||
| ).fail(this.indicate_component_ready.bind(this)); | ||
| } else { | ||
| this.indicate_component_ready(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,36 +1,25 @@ | ||
| from runestone.unittest_base import module_fixture_maker, RunestoneTestCase | ||
| from selenium.webdriver.support import expected_conditions as EC | ||
| from selenium.webdriver.support.ui import WebDriverWait | ||
| from selenium.webdriver.common.by import By | ||
| def _test_poll(selenium_utils, id): | ||
| """ test the poll directive """ | ||
| self = selenium_utils | ||
| self.wait_until_ready(id) | ||
|
|
||
| poll_div = self.driver.find_element_by_id(id) | ||
|
|
||
| setUpModule, tearDownModule = module_fixture_maker(__file__) | ||
| opts = poll_div.find_elements_by_css_selector("input[type='radio']") | ||
|
|
||
| # the poll in overview should be on a scale 1-10. | ||
| assert len(opts) == 10, "Not enough poll options present!" | ||
|
|
||
| class PollTests(RunestoneTestCase): | ||
| def test_poll(self): | ||
| """ test the poll directive """ | ||
| self.driver.get(self.host + "/index.html") | ||
| wait = WebDriverWait(self.driver, 10) | ||
| try: | ||
| wait.until(EC.presence_of_element_located((By.ID, "pollid1_container"))) | ||
| except: | ||
| text = self.driver.page_source | ||
| print(text[:300]) | ||
| # just choose option 4 | ||
| poll_div.find_element_by_id(f"{id}_opt_4").click() | ||
|
|
||
| poll_div = self.driver.find_element_by_id("pollid1_container") | ||
| el = poll_div.find_element_by_id(f"{id}_sent") # check for results span | ||
| assert el.text[:6] == "Thanks" | ||
|
|
||
| opts = poll_div.find_elements_by_css_selector("input[type='radio']") | ||
| # just make sure we can find the results div - an exception will be raised if the div cannot be found | ||
| poll_div.find_element_by_id(f"{id}_results") | ||
|
|
||
| # the poll in overview should be on a scale 1-10. | ||
| self.assertTrue(len(opts) == 10, "Not enough poll options present!") | ||
|
|
||
| # just choose option 4 | ||
| poll_div.find_element_by_id("pollid1_opt_4").click() | ||
|
|
||
| el = poll_div.find_element_by_id("pollid1_sent") # check for results span | ||
| self.assertIsNotNone(el) | ||
| self.assertEqual(el.text[:6], "Thanks") | ||
|
|
||
| # just make sure we can find the results div - an exception will be raised if the div cannot be found | ||
| poll_div.find_element_by_id("pollid1_results") | ||
| def test_poll(selenium_utils): | ||
| selenium_utils.get("index.html") | ||
| _test_poll(selenium_utils, "pollid1") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| # ************************************************************** | ||
| # |docname| - pytest fixtures used by both the client and server | ||
| # ************************************************************** | ||
| # The goal of this test framework is to enable tests written for the Runestone Components to be easily run on the server, where values stored by the database can also be checked. This avoids a duplication of test code between Runestone Components and the server. | ||
| # | ||
| # To accomplish this goal, this file provides functions that work on both client and server. Then, the client and server must each uniquely define two fixtures (see `conftest.py`) specalized for that environment which provided the same results (access to the Selenium WebDriver and to a class of Selenium utilities). Both client and server tests must import from this file; tests can then | ||
| # | ||
| # Structure: | ||
| # | ||
| # - The ``selenium_driver_session`` fixture performs client- and server-specific setup, then returns an instance of a Selenium WebDriver. Since it's specific to the environment, this fixture must be defined differently for client and server. It should be defined at the module level or higher, since starting Selenium is an expensive operation. | ||
| # - The shared `selenium_driver`_ fixture then sets up/tears down the Selenium environment for each test. | ||
| # - The client/server-specific ``selenium_utils`` fixture wraps the shared `_SeleniumUtils`_ class, which provides a common set of methods for the tests. | ||
| # - All client tests should use the ``selenium_utils`` fixture, ensuring that they can run in either environment. | ||
| # | ||
| # Imports | ||
| # ======= | ||
| # These are listed in the order prescribed by `PEP 8 | ||
| # <http://www.python.org/dev/peps/pep-0008/#imports>`_. | ||
| # | ||
| # Standard library | ||
| # ---------------- | ||
| # None. | ||
| # | ||
| # Third-party imports | ||
| # ------------------- | ||
| import pytest | ||
| from selenium.common.exceptions import StaleElementReferenceException | ||
| from selenium.webdriver.common.by import By | ||
| from selenium.webdriver.support.ui import WebDriverWait | ||
|
|
||
| # Local imports | ||
| # ------------- | ||
| # None. | ||
|
|
||
|
|
||
| # Code | ||
| # ==== | ||
| # | ||
| # selenium_driver | ||
| # --------------- | ||
| # Provide a way to reset the state of Selenium for each test, without exiting/restarting the driver (which is slow). | ||
| @pytest.fixture() | ||
| def selenium_driver(selenium_driver_session): | ||
| driver = selenium_driver_session | ||
| # Copied from the Runestone Components test framework. | ||
| driver.implicitly_wait(10) | ||
|
|
||
| yield driver | ||
|
|
||
| # Clear as much as possible, to present an almost-fresh instance of a browser for the next test. (Shutting down then starting up a browser is very slow.) | ||
| driver.execute_script("window.localStorage.clear();") | ||
| driver.execute_script("window.sessionStorage.clear();") | ||
| driver.delete_all_cookies() | ||
|
|
||
|
|
||
| # _SeleniumUtils | ||
| # -------------- | ||
| # Provide basic Selenium-based operations. | ||
| class _SeleniumUtils: | ||
| def __init__( | ||
| self, | ||
| # These are fixtures. | ||
| runestone_selenium_driver, | ||
| host_address, | ||
| ): | ||
|
|
||
| self.driver = runestone_selenium_driver | ||
| self.host_address = host_address | ||
| self.wait = WebDriverWait(self.driver, 10) | ||
|
|
||
| # A helper function to attach to the Selenium driver: get from a URL relative to the Runestone application. | ||
| def get(self, relative_url): | ||
| return self.driver.get( | ||
| "{}/{}".format(self.host_address, relative_url) | ||
| ) | ||
|
|
||
| # Wait until a Runestone component has finished rendering itself, given the ID of the component. | ||
| def wait_until_ready(self, id): | ||
| # The component is ready when it has the class below. | ||
| self.wait.until( | ||
| element_has_css_class((By.ID, id), "runestone-component-ready") | ||
| ) | ||
|
|
||
|
|
||
| # An expectation for Selenium, used for checking that an element has a particular css class. From the `Selenium docs <https://selenium-python.readthedocs.io/waits.html#explicit-waits>`_, under the "Custom wait conditions" subheading. | ||
| # | ||
| # locator - used to find the element | ||
| # | ||
| # returns the WebElement once it has the particular css class. | ||
| class element_has_css_class: | ||
| def __init__( | ||
| self, | ||
| # The element to find; this is passed directly to `driver.find_element <https://selenium-python.readthedocs.io/api.html#selenium.webdriver.remote.webdriver.WebDriver.find_element>`_. See the `Selenium docs`_. | ||
| locator, | ||
| # The CSS class to look for. | ||
| css_class, | ||
| ): | ||
|
|
||
| self.locator = locator | ||
| self.css_class = css_class | ||
|
|
||
| def __call__(self, driver): | ||
| # Find the referenced element. Ignore stale elements. | ||
| try: | ||
| element = driver.find_element(*self.locator) | ||
| if self.css_class in element.get_attribute("class"): | ||
| return element | ||
| except StaleElementReferenceException: | ||
| pass | ||
| return False |
Uh oh!
There was an error while loading. Please reload this page.
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.
Now that I look at this more carefully, I think that the call to
this.csresolver()is solving the same problem as adding the runestone-component-ready class and triggering:runestone:component-ready. The difference is that it is resolving a promise that other objects couldawaitor.thenMaybe using that mechanism is better than adding another one? Note that the
csresolveris pretty new and something I added recently which does not look widely used.... It doesn't feel DRY quite yet?I'm not sure why this didn't occur to me when we were talking about it.
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.
Hmm, interesting. I remember seeing that in the code, but not looking at it very deeply. My thoughts:
awaiton acsresolverpromise rather than the old jQuery emit logic. Are there any components which don't invokecheckServerin the constructor? If so, I'd suggest creating the promise in the constructor, so it can be waited on at any point.runestone-component-readyclass. In the future, I could see adding a CSS style to visually indicate when a component is fully loaded, making this useful for more than just testing.getpollresultsendpoint), so just awaiting oncsresolverdoesn't work for all components.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 just pushed 646d361 to address this. It uses a different promise (
this.component_ready_promise), so that we can distinguish betweencheckServercompeting (with thethis.checkServerCompletepromise) and the component being ready (which differs fromcheckServercompletion for some components, such as poll).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.
checkServer is not used by activecode or any of the non-gradable components. So I think having a separate promise is good, and all components can then use their own logic to ensure that when the component is ready then the component_ready_promise can be resolved. For those that do use checkServer we should ensure that checkServer is resolved before resolving component_ready.
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 agree that we should not use crazy hacks for testing when there is a tried and true method for us to use. Adding a class is fine and as you say may have good benefits later.
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.
Sounds good. Since
checkServer()callsindicate_component_readyonly after the server data is loaded, we're in good shape!