This is an archive of the discontinued LLVM Phabricator instance.

[Flang] test_symbols.py module file fix
ClosedPublic

Authored by ijan1 on Aug 12 2021, 1:51 AM.

Details

Summary

Due to how the LIT deals with module files,
this change stores and runs the scripts in
their own temporary directory to prevent
interference in-between different tests.
It also makes `test_symbols.py` be more
consistent with the other scripts.

Diff Detail

Event Timeline

ijan1 requested review of this revision.Aug 12 2021, 1:51 AM
ijan1 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2021, 1:51 AM

Males sense, thank you for working on this! I've made a couple of small suggestions.

flang/test/Semantics/test_symbols.py
5–11

I'm suggesting a small edit to make it clearer what the input arguments to this script are.

22

I'm suggesting a bit more descriptive variable here. tmp could mean a temporary file, directory, socket etc.

Is there a reason to use lit's suggestion for a temporary directory instead using tempfile.TemporaryDirectory or tempfile.mkdtemp?

flang/test/Semantics/test_symbols.py
44–45
ijan1 added a comment.Aug 12 2021, 7:50 AM

Is there a reason to use lit's suggestion for a temporary directory instead using tempfile.TemporaryDirectory or tempfile.mkdtemp?

I decided to use what's already in the LIT and it also more accurately reflects the behaviour of the bash script. Besides that, I don't see a reason why I couldn't use the tempfile library.

Is there a reason to use lit's suggestion for a temporary directory instead using tempfile.TemporaryDirectory or tempfile.mkdtemp?

I like this suggestion - with tempfile.TemporaryDirectory the tests wouldn't require the extra argument (%t). Is the temporary directory name guaranteed to be unique to every test? That wasn't 100% obvious to me from the docs.

Is the temporary directory name guaranteed to be unique to every test? That wasn't 100% obvious to me from the docs.

%t expands to <builddir>/tools/flang/test/<filename>.tmp. That is, it is unique for every test file, but that same for multiple RUN lines in the file and between make check invocations. However, I think it is better to always have a fresh temporary directory, flang-new does not need to see the leftover files from the previous make check run, those should be independent.

%t expands to <builddir>/tools/flang/test/<filename>.tmp. That is, it is unique for every test file, but that same for multiple RUN lines in the file and between make check invocations. However, I think it is better to always have a fresh temporary directory, flang-new does not need to see the leftover files from the previous make check run, those should be independent.

+1 for this approach, thank you for the suggestion!

ijan1 updated this revision to Diff 366903.Aug 17 2021, 8:00 AM
ijan1 marked 2 inline comments as done.

Updated to use tempfile.TemporaryDirectory. Addressed comments by Andrzej.

This revision is now accepted and ready to land.Aug 17 2021, 8:46 AM
awarzynski accepted this revision.Aug 17 2021, 11:47 AM
This revision was automatically updated to reflect the committed changes.