Refactor compile-related tests to share construction. #4396
Conversation
There was a problem hiding this comment.
Really nice helper!
I had remembered reversing the Testing namespace decision, but I can't find anything in Discord... 🤷🏻 (see inline comment, not blocking)
I'd like to move the implementation of the helper to a source file if we could though. LGTM to land with that.
| // A test helper for compile-related functionality. | ||
| class CompileHelper { |
There was a problem hiding this comment.
Would it be OK to add a source file and moving the inline definitions out-of-line?
| #include "toolchain/testing/yaml_test_helpers.h" | ||
|
|
||
| namespace Carbon::Lex { | ||
| namespace Carbon::Testing { |
There was a problem hiding this comment.
For some reason, when we switched to using the Lex sub-namespace, I thought we had ended up switching how tests work instead of putting them in the Testing namespace. But I can't find such a discussion, only the discussion suggesting putting the tests in Testing as you've switched to here.
Anyways, I'm happy either way, only mentioning it in case there was such a discussion, and I just can't find it but you or someone else remember it.
There was a problem hiding this comment.
I've tracked down discussion here, via https://github.com/carbon-language/carbon-lang/pull/3170/files#r1310772837. I'm updating #3070 to mention this.
There was a problem hiding this comment.
But now I see #3244, I guess there was a little more. Maybe the tests still in Carbon::Testing are actually the mistake?
There was a problem hiding this comment.
Undid this part of the change
toolchain/parse/typed_nodes_test.cpp
Outdated
| } // namespace Carbon::Parse | ||
|
|
||
| namespace Carbon::Testing { |
There was a problem hiding this comment.
The need for this does make me somewhat prefer tests in the component namespace rather than the Testing namespace, and using-declarations for the Testing names... But very weakly.
There was a problem hiding this comment.
Undid this part of the change
|
I'm going to go ahead and merge since I have more work depending on this, but please let me know if you see any issues with the changes (particularly mistakes with NS edits) and I'll make another PR to fix. |
Note in particular that this fixes an issue where SharedValueStore had been shared across files, when they should be per-file. This is only visible when doing multiple compilations in a single test, which was rare before.
This also moves these tests into the Testing namespace. My memory of the various namespacing changes is that we'd generally agreed to have tests in Testing so that we'd see SemIR:: and similar, same as we would in a lot of the implementation.