Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

hannahbast
Copy link
Member

@hannahbast hannahbast commented Aug 4, 2023

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.

Hannah Bast added 2 commits August 4, 2023 21:18
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 hannahbast changed the title End2end test Adding end-to-end test for the QLever UI Aug 4, 2023
Copy link
Contributor

@RobinTF RobinTF left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#!/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
Copy link
Contributor

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):
Copy link
Contributor

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)
Copy link
Contributor

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):
Copy link
Contributor

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)
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Member

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
Copy link
Contributor

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:
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change intended?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants