Skip to content

Added Integration Tests to iiab-install-smoke-test.yml - #329

Merged
holta merged 5 commits into
iiab:masterfrom
Akatama:add_int_tests
Jul 26, 2025
Merged

Added Integration Tests to iiab-install-smoke-test.yml#329
holta merged 5 commits into
iiab:masterfrom
Akatama:add_int_tests

Conversation

@Akatama

@Akatama Akatama commented Jul 24, 2025

Copy link
Copy Markdown

Currently it is only running with Firefox. Since we're just trying to confirm that the IIAB install was successful, I figured running both would just make the workflow even more complicated. However, if we decide that Google Chrome is better to run the tests on, then we can do that instead.

There was some kind of permissions issue - Pytest likes to cache some stuff during the test runs. To be clear, both tests still passed, but I didn't like the warning gumming up the output. Especially if we looked at this a year from now, we might forget that it was just pytest complaining about something. The fix I have put in now is to call pytest with -p no:cacheprovider. The caching wouldn't matter anyway since we are running this as a GitHub Action workflow.

Whatever permissions issue, it appears to be unique to the GitHub Actions runner VM, as I did not run into that warning when I made the test work with sudo back in PR #311

@holta

@holta

holta commented Jul 24, 2025

Copy link
Copy Markdown
Member

@Akatama does this presumably add less than a minute on top of the ~10 minute install?

Related:

@Akatama

Akatama commented Jul 24, 2025

Copy link
Copy Markdown
Author

Yes, it adds like 30 seconds total

@holta

holta commented Jul 24, 2025

Copy link
Copy Markdown
Member

Yes, it adds like 30 seconds total

Great!

@thotmx what do you think of this PR?

@holta holta added the enhancement New feature or request label Jul 24, 2025
@holta
holta requested a review from thotmx July 24, 2025 15:30
@holta

holta commented Jul 24, 2025

Copy link
Copy Markdown
Member

@Akatama can you remind us all how these "2 tests" are being invoked exactly?

And what exactly's being tested?

(Can you explain quick how this is working without obviously invoking .github/workflows/integration-test.yml or a curl command towards testing login etc?!)

image

( Above example output pasted in from the bottom of GitHub Action results https://github.com/iiab/calibre-web/actions/runs/16487275854/job/46614490105?pr=329 )

@Akatama

Akatama commented Jul 24, 2025

Copy link
Copy Markdown
Author

can you remind us all how these "2 tests" are being invoked exactly?

Pytest. You can see the pytest command in the screenshot you sent.

This is the full command:
pytest -p no:cacheprovider -s --splinter-webdriver chrome --splinter-headless

Here are the commands that we are passing to pytest:

-s: means to print the output to the command line
--splinter-webdriver: tells pytest which webdrive to use and therefore which browser to use. Here we have firefox
--splinter-headless: runs the browser in 'headless' mode. Headless mode means the browser runs without the GUI. It results in the test run being a little faster, and we couldn't see the GUI anyway in our workflow.

And what exactly's being tested?

@thotmx Is the one who wrote these tests, but I will describe them to you
Test 1 verifies we can see the calibre-web through the browser and that there are no errors
Test 2 logs in as the default user and verifies that we see what we are supposed to see after we log in.

I recommend looking at the documentation for further details, including how the tests are organized.

Comment thread .github/workflows/iiab-install-smoke-test.yml
@thotmx

thotmx commented Jul 24, 2025

Copy link
Copy Markdown

The PR looks good to me. Can you just confirm the comment related with the virtual environment?

@Akatama

Akatama commented Jul 24, 2025

Copy link
Copy Markdown
Author

The PR looks good to me. Can you just confirm the comment related with the virtual environment?

The virtual environment is created by the IIAB install script (which further calls the IIAB Ansible config). It is done a bit weirdly, but Adam seemed resistant to change it. In short, the venv is created and then calibre-web is cloned inside of it. According to Adam, it has been this way for some time.

Anyway, if I sourced venv/bin/activate and it didn't exist, the workflow would error out. (In this case, since we are already in the venv, it is just /bin/activate)

@thotmx

thotmx commented Jul 24, 2025

Copy link
Copy Markdown

@Akatama

Got it.

But I think the problem with the cache permission could be related.

I think to create a new venv for testing is not bad, considering it would isolate the few testing suite dependencies from any other dependency.

And it would run in the 'scope' of the user running the Github Workflow.

My two cents.

But if you says it's ok as it is. I approve it.

@Akatama

Akatama commented Jul 25, 2025

Copy link
Copy Markdown
Author

@holta @thotmx I tried a couple of things.

First, I tried creating a new venv in /usr/local/calibre-web-py3. This worked fine if you used sudo when creating the venv, but we still had the pytest caching warning.

Next, I tried creating the venv in $GITHUB_WORKSPACE, sourcing that venv, installing the integration-tests-requirements.txt file, then running pytest. Again, it worked fine, but the pytest caching warning still appeared.

So what we would probably need to do is this:

  1. Copy pytest.ini, the folder that contains the tests, and integration-tests-requirements.txt (technically the last one is not required, but you might as well do it if you are copying the other two) to $GITHUB_WORKSPACE
  2. Create the venv in $GITHUB_WORKSPACE
  3. Run the tests from $GITHUB_WORKSPACE

It is definitely doable, but I think it complicates the workflow compared to adding one option to pytest, which is explained in both this PR and in the workflow itself.

Let me know what you think.

@holta

holta commented Jul 25, 2025

Copy link
Copy Markdown
Member

Should this PR be merged in the coming ~24 hours?

Sounds very reasonable?

(@Akatama maybe also hint at where we might go next, building out CI / specific test cases, if you have a moment?)

@thotmx thotmx left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agree, keeping simple at this stage is better considering the change is only an extra parameter.

@Akatama

Akatama commented Jul 25, 2025

Copy link
Copy Markdown
Author

(@Akatama maybe also hint at where we might go next, building out CI / specific test cases, if you have a moment?)

Sure!

  1. Add more tests. This is the main task over adding a new workflow. Right now, our tests only do the simplest possible check, we want tests that add ebooks and videos to different shelves and verify only the people that are supposed to be able to access them can access them. This might end up being a lot of tests, and some of them may be long running tests.

  2. After that is done, the next will be to decide what tests we want in this current workflow. In a lot of situations I have seen, simple, quick tests (usually unit tests) are run for a PR, more complex tests are run after merge or before a release.

However, we sort of expect every PR merge to be its own release. As such, we may just decide to eat the time cost of running a lot of tests for each PR. If we really want to, we still could split it up into smoke tests workflow and all tests workflow and just have them all run before the PR is accepted. Let me know your thoughts @holta @thotmx. The benefit of this splitting is if the smoke test workflow fails we will know something is really wrong and we will know it relatively quickly. The disadvantage of such a strategy is a lot of work is done twice (install IIAB, Firefox and test dependencies).

To be clear, the current test run takes around 30 seconds, but around 26 of those seconds are installing Firefox and installing the test dependencies. So the tests take like ~4 seconds to run total.

@holta

holta commented Jul 26, 2025

Copy link
Copy Markdown
Member

Thanks @Akatama for outlining:

The benefit of this splitting is if the smoke test workflow fails we will know something is really wrong and we will know it relatively quickly.

🍃 🍃 🍃

The disadvantage of such a strategy is a lot of work is done twice (install IIAB, Firefox and test dependencies).

☯️

@holta

holta commented Jul 26, 2025

Copy link
Copy Markdown
Member

Thanks @thotmx for reviewing! 💫

@holta
holta merged commit 4764a61 into iiab:master Jul 26, 2025
1 check passed
@Akatama
Akatama deleted the add_int_tests branch November 5, 2025 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants