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.
hannahbast
force-pushed
the
end2end-test
branch
from
e226439 to
7138636
Compare
RobinTF
left a comment
RobinTF
left a comment
There was a problem hiding this comment.
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.
| #!/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 |
|
|
||
|
|
||
| # Global log with custom formatter, inspired by several posts on Stackoverflow. | ||
| class MyFormatter(logging.Formatter): |
There was a problem hiding this comment.
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.
Please remove the unused code
| sys.exit(1) | ||
|
|
||
|
|
||
| class MyArgumentParser(argparse.ArgumentParser): |
There was a problem hiding this comment.
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.
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.
I think it would be better if selenium was part of requirements.txt
There was a problem hiding this comment.
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.
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.
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
3 participants
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.