Load() loads incorrect extension when specified #37857
Conversation
When the filename contains an extension, the FileFinder will now fail to find the file. There are four options: - filename contains extension + useOnlyExts=True: searches exts + extension (filename) - filename contains extension + useOnlyExts=False: searches only extension (filename) - filename does not contain extension + useOnlyExts=True: searches only exts - filename does not contain extension + useOnlyExts=False: searches exts + extensions (facility)
In the case where search is run with a specified extension in the filename, search for that extension only first. If not found, issue a warning and try again with the other facility extensions + extensions that were passed as an argument. uniqueExtensions.pop_back() avoids running search for specified extension again.
GuiMacielPereira
added
Bug
Corrected if statement after warning from cppcheck
Unit test had failed because I had overlooked the fact that when useExtsOnly=True, the search should fail if the specified extensions are incorrect
This behaviour was not specified in the existing unit tests, but from what I gathered when useExtsOnly=True then only those extensions should be considered, even if an extension is explicitly written in the filename.
Added a few unit tests to cover behaviour that was previously ambiguous. If useExtsOnly=True, then only those extensions should be considered. If useExtsOnly=False, it should find the file whether the extension is correctly specified in the filename or not.
Running the FileFinderTest.h locally doesn't use the mantid properties meant for testing automatically, so the search paths meant for testing are not searched on, which causes some tests to fail. These two lines ensure that the mantid properties for tests are used.
| const std::string propfile = ConfigService::Instance().getDirectoryOfExecutable() + "Mantid.properties"; | ||
| ConfigService::Instance().updateConfig(propfile); |
There was a problem hiding this comment.
Wondering if this will affect other tests?
What properties do you need the config service to have which are stored in Mantid.properties, or is it not set up?
Could you just set the ones you need using the api?
There was a problem hiding this comment.
I thought this could affect other tests, but they all ran fine.
We just need the user directories, everything else is ignored.
I'll just set the user directories using the api, I agree that is safer and clearer
There was a problem hiding this comment.
Oh I see, for loading the data. I know there are test data folders which hold data for unit tests specifically, maybe you can find out how those are typically used
There was a problem hiding this comment.
Well, what is happening is that there is a Mantid.properties file in the bin folder but there is also another properties file in the conda environment folder, which takes priority over the one in the bin folder. When running on Jenkins, the one in the bin folder is used and all is well. When running on my machine, the one in my conda environment is used which causes the missing data directories.
I could set the data directories at the beginning of FileFinderTest by hard coding them relative to the bin directory. But if the structure of the build folder changes, then the test would start failing. I also thought about reading the Mantid.properties file in the bin folder to look for the data directories, but since ConfigService is a singleton, I can't really read the properties file without also updating the properties that the singleton holds, which is what I am already doing anyway with the updateConfig call above (unless, of course, I change the ConfigService class a bit to allow for this).
I couldn't find any other test where the data directories are updated in the config service at the beginning of the test, the closest I got was how the config service is updated several times with updateConfig in the ConfigServiceTest, in the same way that I am doing here.
I think this is the best solution we can do, because it doesn't affect other tests and even when the test is run locally, it doesn't change the properties files after running it (because at the start of workbench the priority defaults to the properties file in the conda environment).
There was a problem hiding this comment.
Or I can just hard code the paths as well, whichever you prefer
Unpacked pair with auto [] and removed unecessary if statements that checked whether strings are empty. Adding empty string or not adding it is the same operation.
Renamed extensions -> facilityExtensions and uniqueExts -> extensionsToSearch
Renamed exts -> extensionsProvided and useExtsOnly -> useOnlyExtensionsProvided
Replaced previous standalone if statements with easier to understand nested if statements.
Description of work
As described in #35661 the Load() algorithm might load an incorrect file extension even when the extension is specified in the file name.
Summary of work
I have maintained the same behaviour for backwards compatibility, the only change I made is that if the file with the specified extension is not found, a warning is issued to let the user know that other extensions will be considered.
Further detail of work
To test:
Run the code in the original issue and check that a warning is displayed in the console log. Load the same file without an extension specified and check that no warning is displayed.
Turn on the debug level on the console log and run the code in the original issue again. Check that at first only the extension specified in the filename is considered and then all extensions are considered.
Reviewer
Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeeperswill take care of it.Gatekeeper
If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.