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

Add comments to test/ci files #50

Merged
merged 5 commits into from
Dec 3, 2024
Merged

Conversation

mfschubert
Copy link
Contributor

@mfschubert mfschubert commented Dec 2, 2024

Hi @polyanskiy --- as discussed in this thread, I'm adding some comments to test and CI files. Here's a quick summary:

  • tests/test_parse.py: this test discovers all .yml files in the database and checks that they can be successfully parsed. It can be invoked using the pytest command, which automatically discovers and runs tests. (The pytest and parameterized python packages must be installed).
  • .github/workflows/test.yml: this file defines a github action which checks out the repository, installs pytest and parameterized, and then runs pytest. The action is configured to run on pull requests and for pushes to the master branch.

I've also added a .github/dependabot.yml file, which will handle automatic security/version updates. For example, when a new version of the github checkout action is available, dependabot will create a PR proposing an update to the test.yml file.

Finally, in creating this PR I noticed that another .yml file error appeared. Going forward, to make the most of the automated test/ci capabilities, I would suggest authoring PRs rather than directly pushing to the master branch. Then, you will have a chance to catch any issues before you merge.

Let me know if you have any questions or if I can help otherwise.

@polyanskiy polyanskiy merged commit c71ee98 into polyanskiy:master Dec 3, 2024
1 check passed
@polyanskiy
Copy link
Owner

Thank you!

I’m not very familiar with the tools you are using, so it might take me a bit of time to figure out how to use them. At the moment, I’m having trouble understanding the output of pytest when I try running it on my local copy of the repository:

c:\Users\polyanskiy\Documents\GitHub\refractiveindex.info-database>pytest
================================================= test session starts =================================================
platform win32 -- Python 3.12.7, pytest-8.3.4, pluggy-1.5.0
rootdir: c:\Users\polyanskiy\Documents\GitHub\refractiveindex.info-database
collected 0 items / 1 error

======================================================= ERRORS ========================================================
________________________________________ ERROR collecting tests/test_parse.py _________________________________________
tests\test_parse.py:35: in
class ParseTest(unittest.TestCase):
tests\test_parse.py:36: in ParseTest
@parameterized.expand(PATHS, name_func=custom_name_func)
C:\Program Files\Python312\Lib\site-packages\parameterized\parameterized.py:594: in parameterized_expand_wrapper
name = name_func(f, "{num:0>{digits}}".format(digits=digits, num=num), p)
tests\test_parse.py:30: in custom_name_func
assert "/database/" in path
E AssertionError: assert '/database/' in 'C:\Users\polyanskiy\Documents\GitHub\refractiveindex.info-database\database\catalog-n2.yml'
=============================================== short test summary info ===============================================
ERROR tests/test_parse.py - AssertionError: assert '/database/' in 'C:\Users\polyanskiy\Documents\GitHub\refractiveindex.info-database\da...
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
================================================== 1 error in 1.87s ===================================================

@mfschubert
Copy link
Contributor Author

Ah interesting, this seems to be a windows vs. linux issue. Linux will use / to indicate subdirectories, while windows seems to use \. We should be able to modify the test so that it works on both OSes. I could try to do this tomorrow, unless you prefer to do so.

@mfschubert
Copy link
Contributor Author

Actually I just made a PR for this: #52

@polyanskiy
Copy link
Owner

Great! It's running now but it fails yaml files with certain Unicode characters (e.g., č):

C:\Program Files\Python312\Lib\encodings\cp1252.py:23: UnicodeDecodeError
============================================================= short test summary info =============================================================
FAILED tests/test_parse.py::ParseTest::test_can_parse_database_data_n2_main_AgGaS2_Jansonas_e_yml - UnicodeDecodeError: 'charmap' codec can't decode byte 0x8d in position 235: character maps to
FAILED tests/test_parse.py::ParseTest::test_can_parse_database_data_n2_main_AgGaS2_Jansonas_o_yml - UnicodeDecodeError: 'charmap' codec can't decode byte 0x8d in position 235: character maps to

@mfschubert
Copy link
Contributor Author

Hmm, could you check which version of python you are using? I am using python 3.10.12 with yaml version 6.0.1. Is there any chance you are using a much older version?

@mfschubert
Copy link
Contributor Author

mfschubert commented Dec 3, 2024

Actually, maybe this could fix it. Swap out line 39/40 with the following:

with open(path, "r", encoding="utf8") as stream:
    yaml.safe_load(stream)

@polyanskiy
Copy link
Owner

That’s it! Adding encoding="utf8" fixed the problem.
Thank you!

@mfschubert
Copy link
Contributor Author

Great! By the way, there seems to be an issue with dependabot, which I cannot debug because I don't have the appropriate permission.

Could you access the "more information" link on this page and paste it here? Then I might be able to tell what is wrong.
https://github.com/polyanskiy/refractiveindex.info-database/actions/runs/12145946006

Thanks!

@polyanskiy
Copy link
Owner

Dependabot couldn't find any dependency files in the directory
Dependabot couldn't find any dependency files in the configured directory

@mfschubert
Copy link
Contributor Author

Ok, this can probably be fixed by updating the dependabot.yml file to remove the package-ecosystem: pip section. It would then read:

# This `dependabot.yml` file handles automatic security/version updates. In this repo,
# it may e.g. update the version of the checkout action used in the `test.yml` action.
# See https://github.com/dependabot/dependabot-core for additional info.

version: 2
updates:
  - package-ecosystem: github-actions
    directory: /
    schedule:
      interval: monthly

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.

2 participants