Skip to content
This repository was archived by the owner on Jun 7, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 0 additions & 24 deletions .travis.yml

This file was deleted.

30 changes: 25 additions & 5 deletions runestone/common/js/runestonebase.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { pageProgressTracker } from "./bookfuncs.js";

export default class RunestoneBase {
constructor(opts) {
this.component_ready_promise = new Promise(resolve => this._component_ready_resolve_fn = resolve)
this.optional = false;
if (opts) {
this.sid = opts.sid;
Expand Down Expand Up @@ -47,7 +48,7 @@ export default class RunestoneBase {
}
// This is for the selectquestion points
// If a selectquestion is part of a timed exam it will get
// the timedWrapper options.
// the timedWrapper options.
if (typeof opts.timedWrapper !== "undefined") {
this.timedWrapper = opts.timedWrapper;
} else {
Expand Down Expand Up @@ -158,13 +159,20 @@ export default class RunestoneBase {
}
return post_promise;
}
/* Checking/loading from storage
/* Checking/loading from storage
**WARNING:** DO NOT `await` this function!
This function, although async, does not explicitly resolve its promise by returning a value. The reason for this is because it is called by the constructor for nearly every component. In Javascript constructors cannot be async!
This function, although async, does not explicitly resolve its promise by returning a value. The reason for this is because it is called by the constructor for nearly every component. In Javascript constructors cannot be async!

One of the recommended ways to handle the async requirements from within a constructor is to use an attribute as a promise and resolve that attribute at the appropriate time.
*/
async checkServer(eventInfo) {
async checkServer(
// A string specifying the event name to use for querying the :ref:`getAssessResults` endpoint.
eventInfo,
// If true, this function will invoke ``indicate_component_ready()`` just before it returns. This is provided since most components are ready after this function completes its work.
//
// TODO: This defaults to false, to avoid causing problems with any components that haven't been updated and tested. After all Runestone components have been updated, default this to true and remove the extra parameter from most calls to this function.
will_be_ready = false
) {
// Check if the server has stored answer
let self = this;
this.checkServerComplete = new Promise(function (resolve, reject) {
Expand Down Expand Up @@ -212,6 +220,18 @@ export default class RunestoneBase {
this.checkLocalStorage(); // just go right to local storage
this.csresolver("local");

@bnmnetp bnmnetp Mar 18, 2021

Copy link
Copy Markdown
Member

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 could await or .then

Maybe using that mechanism is better than adding another one? Note that the csresolver is 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.

Copy link
Copy Markdown
Contributor Author

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:

  • This is really nice. I like the idea of allowing code to await on a csresolver promise rather than the old jQuery emit logic. Are there any components which don't invoke checkServer in the constructor? If so, I'd suggest creating the promise in the constructor, so it can be waited on at any point.
  • From a test perspective, there's no clean way to await a JavaScript promise from Python test code. There's probably a hack (set a global variable in JS then poll for it from Python/Selenium), but since there's already good support for waiting for a class to be present, I see no advantage in doing this. So, I'd suggest keeping the code which adds the runestone-component-ready class. 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.
  • This promise doesn't always tell the whole story -- for example, the poll component does some additional server-side operations (loading from the getpollresults endpoint), so just awaiting on csresolver doesn't work for all components.

Copy link
Copy Markdown
Contributor Author

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 between checkServer competing (with the this.checkServerComplete promise) and the component being ready (which differs from checkServer completion for some components, such as poll).

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Since checkServer() calls indicate_component_ready only after the server data is loaded, we're in good shape!

}

if (will_be_ready) {
this.indicate_component_ready();
}
}

// This method assumes that ``this.componentDiv`` refers to the ``div`` containing the component, and that this component's ID is set.
indicate_component_ready() {
// Add a class to indicate the component is now ready.
this.containerDiv.classList.add("runestone-component-ready");
// Resolve the ``this.component_ready_promise``.
this._component_ready_resolve_fn();
}

loadData(data) {
Expand Down
42 changes: 42 additions & 0 deletions runestone/conftest.py
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)
10 changes: 7 additions & 3 deletions runestone/poll/js/poll.js
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down Expand Up @@ -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;
Comment thread
bjones1 marked this conversation as resolved.
$(this.containerDiv).addClass(this.origElem.getAttribute("class"));
$(this.pollForm).html(
`<span style='font-size: Large'>${this.question}</span>`
Expand Down Expand Up @@ -212,6 +213,7 @@ export default class Poll extends RunestoneBase {
}
$(this.resultsDiv).append(list);
}
this.indicate_component_ready();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

@bjones1 bjones1 Mar 18, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only called in the base class if this.checkServer(event_name, true) is invoked (note the true parameter, which defaults to false if not provided). This component doesn't call it, instead calling checkPollStorage().

Here's the flow of this component, as I understand it:

  • Constructor called
  • Constructor invokes checkPollStorage() (line 29)
  • In checkPollStorage() (line 219):
    • If there's nothing in local storage, indicate the component is ready (line 234).
    • Otherwise, load from the getpollresults endpoint (line 228)
      • On success, invoke showPollResults() (line 157); at the end of this function, indicate the component is ready (line 216)
      • On failure, indicate the component is ready (line 232).

...which is one of the more complex flows I've seen in the components.

}
disableOptions() {}
checkPollStorage() {
Expand All @@ -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();
}
}
}
Expand Down
45 changes: 17 additions & 28 deletions runestone/poll/test/test_poll.py
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")
110 changes: 110 additions & 0 deletions runestone/shared_conftest.py
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
2 changes: 1 addition & 1 deletion runestone/shortanswer/test/test_shortanswer.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def test_sa1(self):
def test_sa2(self):
"""No input. Button clicked"""
self.driver.get(self.host + "/index.html")
sleep(1)
sleep(2)
t1 = self.driver.find_element_by_id("question1")

btn_check = t1.find_element_by_tag_name("button")
Expand Down
Loading