The CVS fetcher isn't covered by self-tests. Where/how should I add them? Should I?

I think tests would be good, see lib/spack/spack/test/(git|hg|svn)_fetch.py for examples. Note that these tests are skipped if the dependencies aren't installed, so you'll need to install cvs in .github/workflows/unit_tests.yaml to actually get coverage.

Docs would also be good. They can be added here: https://spack.readthedocs.io/en/latest/packaging_guide.html#fetching-from-code-repositories

The error is:

2021-04-23T20:57:43.3020151Z   >> 1226    Bus error (core dumped)
2021-04-23T20:57:43.3020728Z      1227    make: *** [Makefile:604: sharedmods] Error 135
2021-04-23T20:57:43.3021347Z      1228    make: *** Waiting for unfinished jobs....

while installing Python. That's unrelated to my pull request.

The last remaining error is from codecov, complaining that part of the code is not covered by tests. That's not true – these lines are definitively executed when running the unit tests. (I recall having to correct errors in some of these lines.)

This is ready to be reviewed, btw. Docs and tests are there as well.

Ping?

Ping?

@scheibelp There has been a development after your review...

It turns out that CVS uses different date formats on different systems. Parsing with strptime doesn't work, and I'm using the python-dateutils package instead. This works fine, but this package is not available for Python 2.6. That leaves three options:

• Develop my own general date parsing function (I don't want to do this)
• Find a similar package that supports Python 2.6 (I couldn't find one)
• Disable testing "checkout by date" on Python 2.6

Note that this only affects testing, CVS support itself doesn't require this package.

I've implemented the third with a few if statements. See the beginning of conftest.py:

if sys.version_info >= (2, 7):
    # This package does not exist for Python <2.7. That means that we cannot
    # test checkouts "by date" for CVS respositories. (We can still use CVS
    # repos with all features, only our tests break.)
    from dateutil.parser import parse as parse_date
else:
    def parse_date(string):
        return None

Please let me know whether this is fine.

I've converted this to a draft since I didn't want this to be merged automatically.

I also prefer option 3 of #23212 (comment), although my preference is that this be implemented with pytest.mark.skipif on the test itself (if that's hard to do as a decorator because of parameterization, I think git_fetch has an example of pytest.skip inside of the unit test.

On the other hand, that separates the skip from the reason (i.e. the construction in the fixture in conftest, so I'm wondering if you could conditionally define a parse_date that looks like:

def parse_date(string):
    pytest.skip("Not available for Python 2.6")

And it also occurs to me it might be worth mentioning that CVS outputs different date formats for different systems in a comment related to this logic.

Closing/reopening to restart pipeline

Thanks!

I see this PR introduced a dependency on python-dateutil at least to run tests, but if I am not missing something it doesn't vendor the corresponding package. This means that before this PR one could git clone Spack and run:

$ spack unit-test

while after this PR we need to:

$ pip install python-dateutil

before running any unit test locally. Is this wanted?

@alalazo I don't know what "to vendor" means.

Yes, one now needs to install python-dateutils as well – I didn't realize that this could lead to trouble. I guess it would be possible to test whether this package is installed, and if not, skip the respective tests with a warning?

I don't know what "to vendor" means.

That's a good definition of what I mean by "vendor". We have a few 3rd party libraries shipped with Spack in lib/spack/spack/external.

I guess it would be possible to test whether this package is installed, and if not, skip the respective tests with a warning?

Should be hopefully solved by #24484 thanks!