Ideally should avoid hardcoded file path in tests

@taldcroft

Description

I see things in the tests like:

ROOT = os.path.abspath(os.path.dirname(__file__))
t_comp = read(os.path.join(ROOT, filename))

Is there a reason why this is not used instead?

t_comp = read(get_pkg_data_filename(filename))

There is also a variation using DATA global variable.

One of the subpackage where I saw this was io.ascii, so cc @taldcroft and @hamogu

It's mostly because the io.ascii code predates astropy. get_pkg_data_filename was not around in those days. We've never had problems with this, so I don't think it's worth anybodies time to change all the tests for negligible benefit unless you are aware of a situation where the current version will fail for some legal way of running the tests (e.g. some tox setting, headless servers, etc. - I don't think any of these apply here though).

I like consistency but I have no strong opinion here since I think this only affects some tests and they are passing.

Could be useful to have a comment to this effect somewhere, so people writing new tests are reminded to use get_pkg_data_filename where possible. But these cases are scattered over at least have a dozen files, so not sure where to best place it.

It is already documented in https://docs.astropy.org/en/latest/development/testguide.html#working-with-data-files but I don't know who reads it.

Hi humans 👋 - this issue was labeled as Close? approximately 7 days ago. If you think this issue should not be closed, a maintainer should remove the Close? label - otherwise, I will close this issue in a month.

If you believe I commented on this issue incorrectly, please report this here

I definitely like consistency and bringing code to a certain standard within the project. If anybody feels motivated then I will happily approve! I lot of new code gets created on the basis of existing code, not the documentation, (speaking from personal experience!) so there is value in making the changes.

@taldcroft

I'm with @taldcroft here and for io.fits in particular feel that the fact that the code has a somewhat deviating standard is one of the things it makes it a bit more difficult to work on. (Absolutely no criticism implied, just that consistency is very helpful!)

Since I think nobody is arguing against changing it to make it more standard, I removed labels to leave this as a low-priority issue.

Re: io.fits -- That one has a lot of history and was basically copy-pasted from PyFITS. 😬

Hey guys,
I got some free time for now, mind if I try and tackle this?

@iamsoto

Go for it, @iamsoto ! Hopefully some of them are already fixed on master since, but I am sure there are some left.

Cool!
Just for clarification, are we using the get_pkg_data_filename or get_pkg_data_fileobj for tests that conduct readonly operations? Since looking at the docs, they appear to throw exceptions if the file is not found. So I imagine tests that write to files probably need to keep their hardcoded file paths.

For example, I found this chunk of code in /coordinates/tests/accuracy/generate_ref_ast.py

    f = open(os.path.join('data', fnout), 'wb')
    f.write("# This file was generated with the {} script, and the reference "
            "values were computed using AST\n".format(os.path.basename(__file__)))

@iamsoto

@iamsoto - I think that particular piece of code is not executed during tests, but is rather there so that we know how the test data were generated (and can regenerate them if AST ever changed significantly). So there would be no need to change that.

More generally, any temporary file writing should be done using the tmpdir fixture, but I'm fairly sure that is generally the case - definitely do ask if you see something else that seems odd!

Navigation Menu

Repository navigation

Ideally should avoid hardcoded file path in tests #10344

Description

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Ideally should avoid hardcoded file path in tests #10344

Description

Description

Activity

hamogu commented on May 12, 2020

pllim commented on May 12, 2020

dhomeier commented on May 13, 2020

pllim commented on May 13, 2020

astropy-bot commented on May 21, 2020

taldcroft commented on May 27, 2020

mhvk commented on May 27, 2020

pllim commented on May 27, 2020

i-am-b-soto commented on Jun 23, 2020

pllim commented on Jun 23, 2020

i-am-b-soto commented on Jun 23, 2020

mhvk commented on Jun 24, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions