Tests fail when running the full testing suite by HarmonicReflux · Pull Request #365 · EnergySystemsModellingLab/MUSE_OS

HarmonicReflux · 2024-06-21T16:03:30Z

Description

This pull request refactors the way muse --model -default is executed to ensure that all tests pass.

Specifically, the Results/MCACapacity.csv file is now placed within the docs folder, which is where the test expects to find MCACapacity.csv. The necessary file is created during the test run by the notebook itself and subsequently picked up by it. This change resolves the error that causes pytest to fail, and as a result, all tests now pass by default.

Fixes #261

Type of change

Please add a line in the relevant section of
CHANGELOG.md to
document the change (include PR #) - note reverse order of PR #s.

New feature (non-breaking change which adds functionality)
Optimization (non-breaking, back-end change that speeds up the code)
Bug fix (non-breaking change which fixes an issue)
Breaking change (whatever its nature)

Key checklist

All tests pass: $ python -m pytest
The documentation builds and looks OK: $ python -m sphinx -b html docs docs/build

Further checks

Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

…"easy" fix regarding the test that makes the .ipynb notebook fail, I included `muse --model default` to be run as a necessary step to complete setting MUSE_OS up. Given the fact that after patching the setup, 304 tests pass successfully while 10 are skipped, 1 comes with an expected fail and 4 warnings , it is probably better to sort these out in separate issues to separate responsibilities and make it easier to solve them.

…which is true only upon running `muse --model default` or any other model that creates MCACapacity.csv and only then proceeds running the test notebooks. I hard-code this explicit file dependency which may not be the most general solution, however, due to the explicit dependency on MCACapacity.csv in one of the test notebooks, which itself is hardcoded, I think this is a straight-forward approach.

codecov · 2024-06-21T16:13:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.39%. Comparing base (2e8e1b5) to head (297e8db).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #365      +/-   ##
===========================================
+ Coverage    71.36%   71.39%   +0.03%     
===========================================
  Files           44       44              
  Lines         5915     5915              
  Branches      1162     1162              
===========================================
+ Hits          4221     4223       +2     
+ Misses        1371     1370       -1     
+ Partials       323      322       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…suite

dalonsoa

I've re-run the failed test and now things work as planned. It is just a flaky test.

The changes look good and the explanations are clearer now, so all good.

For future PR remember to 1) provide a description of what the PRs is about, 2) indicate what issue it is closing and 3) make sure the branch is up to date.

dalonsoa · 2024-06-24T12:00:16Z

docs/installation/developers.rst

+    # 1- Create a virtual environment
+    # 2- Activate that virtual environment
+    # 3- Install MUSE in editable mode with: python -m pip install -e .[dev,doc]
+    # 4 - Invoke `muse --model default`


So this is the key thing to enable all tests to pass when the full suite is run at once, right?

I don't think so, as this will generate a results folder in the current working directory, whereas we specifically need it in the docs folder

In the docs workflow a link is made between both, but it is true I'm not entirely sure that's a solution in this case.

Yes. For the tests to pass without the failure one has to run muse --model default.
Independently, there are is still an "expected fail", some tests that got "skipped" and four "warnings", though I opened separate issues for them.

In the docs workflow a link is made between both, but it is true I'm not entirely sure that's a solution in this case.

Yeah, that was a hack on my part to avoid having to generate results twice.

I think it's a good idea to tell users that they need to run this command in order for tests to pass, but I think we should put that in the "Running Tests" section, not here.

tsmbland · 2024-06-24T12:14:52Z

tests/test_docs_notebooks.py

 def available_notebooks() -> list[Path]:
    """Locate the available notebooks in the docs."""
+    if not Path("Results/MCACapacity.csv").exists():
+        return []


Is this saying that it will run no notebook tests if that path doesn't exist? I don't think we want that

Sorry, I missed this file. Yes, we do not want this. All tests should run.

dalonsoa

All tests should run, and not skipping them if a file is missing. If it is missing, we need to figure out why it is missing and put it in there.

dalonsoa · 2024-06-24T12:16:22Z

tests/test_docs_notebooks.py

 def available_notebooks() -> list[Path]:
    """Locate the available notebooks in the docs."""
+    if not Path("Results/MCACapacity.csv").exists():
+        return []


Sorry, I missed this file. Yes, we do not want this. All tests should run.

HarmonicReflux · 2024-06-24T15:16:48Z

Well, for "Results/MCACapacity.csv" to be visible, one has to create it first, and the most simple way of creating it is by running a model, say the default model via muse --model default. Hence, I included invocation of this command in the documentation. If users then run the test suite, the failure will not occur.

As @dalonsoa pointed out: If my suggestion is not the one to go for, I can think of either:
i) Include the file into the codebase (still feels a bit hacky to me)
ii) Defining to tun the default model in the setup file such that it is created while running python -m pip install -e .[dev,doc]
iii) Any other suggestions?

dalonsoa · 2024-06-25T06:02:24Z

I think the main issue here is:

to figure out what notebook needs the Results folder in the root folder
then take the steps such that the data it looks for is within the docs directory - like any other of the notebooks - and put the data in the right place.
And yes, committing the data as well, as it is done with the other notebooks that need data, like those of the tutorials.

What makes no sense is to have a notebook in the docs requiring data in the root directory.

There's a discussion going on on how to avoid committing data to the repository, but that's a longer term discussion and we do not have an answer for it, yet.

dalonsoa · 2024-06-25T06:02:59Z

And remember to update the branch and, ideally, add a PR description - not essential, but serve as reference for the future.

alexdewar · 2024-06-25T09:19:47Z

Did you want me to review this @HarmonicReflux?

HarmonicReflux · 2024-06-28T14:48:50Z

Did you want me to review this @HarmonicReflux?

Yes, please take a look, so we are all on the same page.
Thks.

alexdewar

As the others have said, we don't want to disable tests when the files aren't there. While that will make the tests pass, that'll just mean that developers who haven't run the default model first won't run these tests at all, so they won't know if their code is broken until they open a PR.

I think the right solution is to just put an instruction about this in the documentation for now. We could potentially do something cleverer, like automatically running the default model in conftest.py if the files are missing, but we shouldn't do that on this PR. I'd maybe open an issue called "default model must be run for tests to pass" and we can have a think about it. (We don't want to go back to committing the contents of Results/ to the repo, because we've only just got rid of them!)

As @dalonsoa has said, you want a PR description so reviewers know what has been changed and why. In this case, it would have been good to have a heads-up that you'd disabled some of the tests! If @tsmbland hadn't clocked it then it could have broken things down the line.

alexdewar · 2024-06-28T15:29:40Z

docs/installation/developers.rst

+    # 1- Create a virtual environment
+    # 2- Activate that virtual environment
+    # 3- Install MUSE in editable mode with: python -m pip install -e .[dev,doc]
+    # 4 - Invoke `muse --model default`


In the docs workflow a link is made between both, but it is true I'm not entirely sure that's a solution in this case.

Yeah, that was a hack on my part to avoid having to generate results twice.

I think it's a good idea to tell users that they need to run this command in order for tests to pass, but I think we should put that in the "Running Tests" section, not here.

Revoked changes to exclude the test in case input file is not there as per discussion for pull requests. Updated the manual to instruct users to run the default model on Muse to complete the installation. This ensures all tests pass (some may be skipped) and avoids including model results files in the codebase.

HarmonicReflux · 2024-07-01T11:38:12Z

Appreciated comments of the reviewers.

…suite

alexdewar

I still think the text about having to run muse --model default should be in the "Running Tests" section. See comment

…l before the test scripts. Running a model is now mentioned twice: once in the installation instructions and once before running the tests. This ensures readers who do not follow the guide in sequence will understand the requirement and avoid confusion.

…suite

HarmonicReflux · 2024-07-01T18:53:17Z

@alexdewar please take a look at this reworked pull request.

tsmbland · 2024-07-02T10:02:50Z

I'm still a bit unsure about this, because we specifically need a results folder in the docs folder, so running muse --model default in the root folder won't be enough. The link that Alex created in the docs workflow will only exist if the user runs the documentation build locally (right?) which we're not asking people to do before running the tests, so I'm not sure how this fixes the problem. Am I missing something?

HarmonicReflux · 2024-07-02T15:40:22Z

Running a model will create a "Results" folder in the directory MUSE is installed. The results folder itself then contains subfolder that of which one contains the file the one testing the notebooks needs to have access in order to run successfully. To my knowledge, building the documentation is a separate process unrelated to the tests themselves.

tsmbland · 2024-07-02T15:45:13Z

I get that, but the notebook in question (running-muse-example.ipynb) points to a Results folder contained in the docs folder, not the root folder

…suite

…file that is required for the notebook tests.

…installation procedure as required file creation to run notebook tests is now fixed.

for more information, see https://pre-commit.ci

HarmonicReflux · 2024-07-08T15:18:11Z

@dalonsoa and @tsmbland,
please review this pull-request.

@alexdewar is out of office until later this week.

tsmbland

Looks good! Just a small change needed which I didn't spot before

docs/installation/developers.rst

Co-authored-by: Tom Bland <t.bland@imperial.ac.uk>

…suite

dalonsoa

The latest modification is very neat! Let's hope it works in all instances we are interested.

alexdewar

LGTM!

HarmonicReflux added 2 commits June 20, 2024 14:15

HarmonicReflux requested a review from dalonsoa June 21, 2024 16:04

Merge branch 'develop' into tests_fail_when_running_the_full_testing_…

98a0fb8

…suite

dalonsoa approved these changes Jun 24, 2024

View reviewed changes

tsmbland reviewed Jun 24, 2024

View reviewed changes

dalonsoa requested changes Jun 24, 2024

View reviewed changes

HarmonicReflux requested a review from alexdewar June 28, 2024 14:48

alexdewar requested changes Jun 28, 2024

View reviewed changes

HarmonicReflux requested review from alexdewar, dalonsoa and tsmbland July 1, 2024 11:38

Merge branch 'develop' into tests_fail_when_running_the_full_testing_…

5456ac7

…suite

alexdewar requested changes Jul 1, 2024

View reviewed changes

HarmonicReflux added 2 commits July 1, 2024 19:14

Merge branch 'develop' into tests_fail_when_running_the_full_testing_…

e4ce98a

…suite

HarmonicReflux added 2 commits July 8, 2024 13:52

Merge branch 'develop' into tests_fail_when_running_the_full_testing_…

2cbddc2

…suite

Split markdown cell while executing default model to create capacity …

e16cf54

…file that is required for the notebook tests.

HarmonicReflux and others added 3 commits July 8, 2024 15:11

Deleted instructions to run the default model of muse as part of the …

4cc1e8e

…installation procedure as required file creation to run notebook tests is now fixed.

Improved user documentation.

9b08706

[pre-commit.ci] auto fixes from pre-commit.com hooks

eca80f1

for more information, see https://pre-commit.ci

HarmonicReflux self-assigned this Jul 8, 2024

tsmbland requested changes Jul 8, 2024

View reviewed changes

docs/installation/developers.rst Outdated Show resolved Hide resolved

docs/installation/developers.rst Outdated Show resolved Hide resolved

docs/installation/developers.rst Outdated Show resolved Hide resolved

docs/installation/developers.rst Outdated Show resolved Hide resolved

HarmonicReflux requested a review from tsmbland July 8, 2024 15:30

HarmonicReflux and others added 3 commits July 8, 2024 16:31

Update docs/installation/developers.rst

f00917d

Co-authored-by: Tom Bland <t.bland@imperial.ac.uk>

Update docs/installation/developers.rst

f5ead55

Co-authored-by: Tom Bland <t.bland@imperial.ac.uk>

Update docs/installation/developers.rst

20d3e5e

Co-authored-by: Tom Bland <t.bland@imperial.ac.uk>

tsmbland approved these changes Jul 8, 2024

View reviewed changes

Merge branch 'develop' into tests_fail_when_running_the_full_testing_…

297e8db

…suite

dalonsoa approved these changes Jul 10, 2024

View reviewed changes

alexdewar self-requested a review July 11, 2024 08:18

alexdewar approved these changes Jul 11, 2024

View reviewed changes

HarmonicReflux merged commit bca773a into develop Jul 12, 2024

HarmonicReflux deleted the tests_fail_when_running_the_full_testing_suite branch July 12, 2024 09:12

Comments

Conversation

HarmonicReflux commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Key checklist

Further checks

Uh oh!

codecov bot commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dalonsoa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalonsoa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HarmonicReflux commented Jun 24, 2024

Uh oh!

dalonsoa commented Jun 25, 2024

Uh oh!

dalonsoa commented Jun 25, 2024

Uh oh!

alexdewar commented Jun 25, 2024

Uh oh!

HarmonicReflux commented Jun 28, 2024

Uh oh!

alexdewar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HarmonicReflux commented Jul 1, 2024

Uh oh!

alexdewar left a comment

Choose a reason for hiding this comment

Uh oh!

HarmonicReflux commented Jul 1, 2024

Uh oh!

tsmbland commented Jul 2, 2024

Uh oh!

HarmonicReflux commented Jul 2, 2024

Uh oh!

tsmbland commented Jul 2, 2024

Uh oh!

HarmonicReflux commented Jul 8, 2024

Uh oh!

tsmbland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dalonsoa left a comment

Choose a reason for hiding this comment

Uh oh!

alexdewar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

HarmonicReflux commented Jun 21, 2024 •

edited

Loading

codecov bot commented Jun 21, 2024 •

edited

Loading