-
Notifications
You must be signed in to change notification settings - Fork 16
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
Adding end-to-end test for the QLever UI #56
base: master
Are you sure you want to change the base?
Conversation
So far, the QLever UI code has zero continuous integration tests. The first step is to add a test that checks that the page is shown propery and some basic functionality.
e226439
to
7138636
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.
There are a couple of things here that need fixing, but on a general level the idea is not bad as a starting point.
I'd like to suggest TypeScript
as part of the testing toolchain so that silly errors with types can already be caught when writing the code.
If you're still on the fence about introducing a "compilation step" into this code base, JSDoc might just be the compromise you're searching for. Basically you'd put type annotations into function documentation and run the TypeScript compiler on the JS file afterwards to ensure the types are all correct.
There are some limits to this approach, and it also doesn't solve the issue of a missing module system (currently a lot of the code just expects some global fields to be there, which are defined in other files), but I'd be happy to talk with you about this.
@@ -0,0 +1,157 @@ | |||
#!/usr/bin/python3 |
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.
#!/usr/bin/python3 | |
#!/usr/bin/env python3 |
So it works with virtualenvs
from selenium import webdriver | ||
from selenium.webdriver.firefox.options import Options | ||
from selenium.webdriver.firefox.firefox_binary import FirefoxBinary | ||
# from selenium.webdriver.chrome.options import 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.
Unused import
|
||
|
||
# Global log with custom formatter, inspired by several posts on Stackoverflow. | ||
class MyFormatter(logging.Formatter): |
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.
MyFormatter
should be renamed to something that doesn't scream coding tutorial :D
self.driver = webdriver.Firefox(options=options) | ||
# self.driver = webdriver.Chrome(options=options) | ||
# self.driver.set_window_position(100, 0) | ||
# self.driver.set_window_size(1400, 600) |
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.
Please remove the unused code
sys.exit(1) | ||
|
||
|
||
class MyArgumentParser(argparse.ArgumentParser): |
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 here, the class should be renamed
log.setLevel(logging.INFO) | ||
handler = logging.StreamHandler() | ||
handler.setFormatter(MyFormatter()) | ||
log.addHandler(handler) |
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'd suggest you move this inside the if __name__ == '__main__'
block.
This also makes clear that the level is set twice
sudo apt-cache policy docker-ce | ||
sudo apt install docker-ce containerd.io | ||
# Install packages for use of Selenium | ||
pip3 install 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.
I think it would be better if selenium was part of requirements.txt
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.
Perhaps a requirements-test.txt
for test requirements. This way users only have to install the necessary packages but the testing requirements are still much more visible.
try: | ||
self.driver.close() | ||
except Exception: | ||
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.
Why would we ignore any exceptions here?
log.info(f"Page {self.url} loaded successfully") | ||
break | ||
except Exception as e: | ||
if i < self.num_retries - 1: |
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'm not sure if I like this retry mechanism. Ideally the timeout should be long enough so that it always works. If it casually fails that just means the timeout is too slow in my opinion
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 this change intended?
So far, the QLever UI code has zero continuous integration tests. The first step is to add a test that checks that the page is shown properly and that the basic functionality works.