[CALCITE-3595] Test infrastructure overwrites reference log with wrong results - #1671
[CALCITE-3595] Test infrastructure overwrites reference log with wrong results#1671yanlin-Lynn wants to merge 1 commit into
Conversation
| new File( | ||
| Sources.of(refFile).file().getAbsolutePath() | ||
| .replace("test-classes", "surefire")); | ||
| .replace(".xml", "_surefire.xml")); |
There was a problem hiding this comment.
hi @vlsi , I'm not sure I fully understand your question. I try to explain the logic of this PR.
I think the original intention was to create a log file in "surefire" directory, containing the actual relnode tree for sql after run test.
But now, the ref file is not in "test-classes" directory (no test-classes directory exist), make log file and ref file become the same file. So update log file will update ref file too.
This pr just to make sure log file and ref file are different files.
There was a problem hiding this comment.
I suggest we use a human-understandable naming.
For instance: ${name}.actual.xml. Is it the file with the actual output? WDYT?
Frankly speaking, it is puzzling why xml file is called a log file, but if it works who cares.
There was a problem hiding this comment.
We should at least add a check here that the charSequence to replace must exists, or, the log file path and ref file path must be different, to avoid such problems when we do later code upgrade.
There was a problem hiding this comment.
@danny0405 , for this case ".xml" is sure to exists, the code final URL refFile = findFile(clazz, ".xml"); guarantee that. In case code upgrade in future, I'll add an check to make sure re file and log file are different.
@vlsi I'm OK with both names. I'll update the PR as you suggested.
…g results (Wang Yanlin)
10450cb to
8b49e62
Compare
danny0405
added
the
LGTM-will-merge-soon
|
Huge +1. Thanks for debugging it! |
…g results (Wang Yanlin) close apache#1671
…g results (Wang Yanlin) close apache#1671 Change-Id: I53d54c677970ba44a91dd9e6127a38f304a399da
…g results (Wang Yanlin) close apache#1671 Change-Id: I53d54c677970ba44a91dd9e6127a38f304a399da
5 participants
See jira CALCIT-3595