space-station-14
space-station-14 copied to clipboard
space-wizards
Content unit tests refactor
About the PR
Content.Tests (unit tests for Content) needed some love. No file was left untouched.
- All warnings resolved (with the exception of two
[Skip]ped tests, more on that later), - All hints applied, including ones from C# 12.0,
- Every file is now in file-scoped namespace,
- Consistent attributes everywhere:
-
[TextFixture]features on all Test Fixtures, -
[TestOf]features on all Test Fixtures, -
[Parallelizable]features on all Test Fixtures, - All of the above in one line
- Redundant
[Test]s removed from[TestCase]methods,
-
- Everything should now run in parallel in various degrees (some down to Fixtures, some down to Test Cases) as allowed by existing logic,
- Minor data/logic refactoring to make things more parallelizable & memory efficient.
Why / Balance
- Speed up test runs. 2. Get rid of warnings and hints. 3. Improve standard of code.
Technical details
Everything that was changed is in About the PR. What wasn't changed is I was unable to make the two [Skip]ped Alerts' tests working. I took a gander at fixing ExtraSystemLoading in RobustUnitTest, and with some decent success. Ultimately I was unable to: 1.) Get ContentUnitTest IoC to load with Project set to client. There's two dependencies that dislike being loaded at that point of startup and will not cooperate. 2.) Reconcile the order of initializing all dependencies. In particular EntitySystems that depend on prototypes being loaded want to be loaded after ISerializationManager, but current implementation wants to load ISerializationManager after them.
I might give it a second shot some other time.
Media
No media. Pure tests refactor.
- [X] I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase
Applying code style changes like file scoped namespaces to existing code just churns the code, causes conflicts, breaks git blame, and provides no further benefit.
This stands in direct contradiction with what The Robust Book states. It is plainly recommending making PRs that specifically change namespaces to file-scoped in two separate places: as one of the conventions and as an example of a refactor PR (the latter being a change of existing old code).
The benefits are: fewer warnings, adhering to agreed-upon code conventions, codebase consistency. Git blame stands unbroken, avoiding conflicts shouldn't hinder code improvement and I do not understand what you mean by "churning the code".
Do you want no style changes at all? What about the 100+ code style warnings? Why have editor.config mark formatting as warnings in the first place if we do not want them fixed? I'm confused.
Of course I will comply. I just find it strange for a code complying with The Robust Book and editor.config to be non-compliant, since that is supposed to literally BE the source of defining what is compliant (?). Tell me what needs to be changed.
This stands in direct contradiction with what The Robust Book states. It is plainly recommending making PRs that specifically change namespaces to file-scoped in two separate places: as one of the conventions and as an example of a refactor PR (the latter being a change of existing old code).
I'm gonna ask the rest of the maintainers what our stance is here.
The benefits are: fewer warnings
If there are actual compiler warnings being fixed then that's fine. But style warnings in Rider really are the least of our priorities right now. If we want any sort of enforcement for those we'd need to actually enforce them via CI.
Git blame stands unbroken
If I have to dig into this code via various history/blame tools I now have to dig through more layers of commits to see what they did.
avoiding conflicts shouldn't hinder code improvement
The code is barely improved. Changing something to file-scoped namespaces does not make it any easier to read, maintain, etc...
Do you want no style changes at all? What about the 100+ code style warnings? Why have editor.config mark formatting as warnings in the first place if we do not want them fixed? I'm confused.
I kinda just hope that people actually bother to look at the squiggles in their IDE when they're writing stuff.
Also a lot of the warnings and hints you're fixing are stuff like the NUnit analyzer which, yeah, it's nice, but also adding Assert.Multiple() all over the place isn't gonna help anybody.
I'm gonna ask the rest of the maintainers what our stance is here.
Personally if a file is getting touched I don't like file-scoped namespaces unless the file is like 50+% re-written to the point you pretty much need to read it as a new file.
Sorry for a wall of text. I promise all of it is on topic.
Let me explain my motivation here
This is a large and open project. The size makes it so there is a lot of code that is of poor quality and this includes code style as well (we will get to how poor in a minute). It being an open project also means a lot of contributors are newcomers coming with no prior knowledge about the codebase (conventions and architecture), with a sizeable portion of them dabbling in C# for the first time.
What do people usually do when they want to implement a functionality similar to an already existing one? They check how the similar functionality works in the existing code and replicate it (which undeniably usually comes to copy-pasting, evidence for which is aplenty). This applies to both logic but also to code style.
And the spotlight isn't just on new contributors. There are a lot of people that have been involved with the project for a while doing that as well because the project is big and is changing at a fast pace. It's difficult and a major hassle to keep track of everything when all you want to do it just add one small feature. [Obsolete] attribute helps with that, so do analyzers and other tools, but there's just a lot of code and a lot of places will always be "lagging behind" in code standard.
And thus people frequently end up: a) looking at existing code with bad code standards as an example of how things should be done, and b) directly copying bad code standards from existing code into new code.
I'm no exception and find myself doing that myself, because I work under the assumption that since the old code works, if I were copy the way old code does things, then my new code will also work. Which is only reasonable. And since the full code conventions are not written anywhere, same assumptions seem reasonable for code style. (The Robust Book tackles the topic nicely, but it itself also isn't updated frequently enough).
In the long run this leads to bad code standards proliferation which is additional burden for reviewers to catch, and slows down the contribution cycle in general (adds change requests that otherwise wouldn't happen). If not caught by reviewers it will lead to more proliferation since that will merge new code that will serve as a bad example for others to copy from. I've seen a number of PRs here asked for changes just on grounds of code standard being not up to spec. (Huge thank you to all reviewers for doing this!) Most of the time the cause for that was because it was copied from another, old piece of code.
There is an undeniable benefit to revisiting old code, even for small seemingly insignificant changes ("code churning"?). One example I want to bring up is removing name parameters from DataFieldAttributes where they match with auto-generated names. This is an exclusively code standard change that - if applied indiscriminately to all components in the projects - would stop that mistake from being replicated in new code ever again (by new contributors and old alike).
Cleaning up the codebase with no material changes goes a long way for project's longevity. And I'm sure reviewers would also appreciate whatever load they can get off their back (people copying less bad style from old code). This PR is just a small step in this process, but arguably it's a step in the right direction.
Regarding this PR in particular
If we want any sort of enforcement for those we'd need to actually enforce them via CI.
In most projects .editorconfig serves as means of enforcing code style rules. It can be fully permissive (ignore), suggestive (show in editor but not as hint), soft enforcement (hint), stronger enforcement (warning), or hard enforcement (error). Granularity is very fine (individual for each piece of code style).
This is why I made the code style adjustments that I did in this PR - I was under the impression that .editorconfig was being used in this manner: as means to soft-enforce a consistent and agreed-upon style of code.
If that was a wrong assumption on my part then I will undo the changes, however the point stands that .editorconfig is usually used for this purpose and it being incorrectly set (if that happens to be the case) will mislead other developers working on the project. It should be adjusted accordingly.
Proposed resolution: Need confirmation whether .editorconfig is set correctly, and if not it needs to be adjusted.
adding
Assert.Multiple()all over the place isn't gonna help anybody
Agreed. I, too, dislike that NUnit hint. The Assert.Multiple() is for specialized cases where you want to run all of the asserts before reporting a failure (i.e. for more verbose failure reporting). In majority of cases it's overkill, as we expect all asserts to succeed and the extra information is rarely of any use. IMO it should be the decision of the person writing the tests whether the extra verbosity is required, or fail-on-first-assert-failure is the expected behavior.
Proposed resolution: Add dotnet_diagnostic.NUnit2045.severity = none to .editorconfig.
Changing something to file-scoped namespaces does not make it any easier to read,
I worked on a few projects where people approached the topic with similar skepticism so apologize if this sounds like a "rehearsed answer" - this is because it is one.
File-scoped namespaces actually help quite a bunch by shifting indentation of the whole file several characters to the left. The smaller benefit is that it makes the code slightly more readable (one scope { } less to think about), but the main argument for using file-scoped namespaces is that is saves horizontal space.
We want to avoid making obscenely long lines of code because code readability suffers. A lot of project adopt conventions that suggest limiting the number of characters per line to some bounds. The main idea here is so that when viewing the code in the editor all of the lines of code fit within the editor width and the chore of using a horizontal scrollbar can be avoided.[^1] Usually the numeric suggestions fall within 100-150 characters and aren't strictly enforced. The Robust Toolbook gives one suggestion on this topic, but provides no numeric limit to stay within.
Regardless of the convention, in a non-file scoped namespace, every line of code must be that little bit of indentation width shorter in order to fit on an editor screen without scrolling. Freeing up those spaces (usually 4) effectively increases the length of a line that still fits in the same-width editor[^2] and improves flexibility of line breaks for long lines. Just the fact that all lines of code fit in the editor width is a huge benefit in and of itself. IMO it is worth it, but I am just one voice and I don't want to be "that guy" pushing ideas on others.
Proposed resolution: Make a decision how much do we want to have file-scoped namespaces. Do we want any sort of enforcement (.editorconfig is capable of that), or perhaps we want a guideline (in The Robust Book) suggesing people leave it the way it is unless they are doing a major refactor of that file (metalgearsloth's suggestion from the comment).
[^1]: You may think that wide- and ultrawide screens alleviate the issue, but on the contrary it is quite common for users with these screens to instead split the editor into two editor-columns horizontally, once again making screen space width a limiting factor.
[^2]: It's almost like having a wider screen!
All in all, I will adjust this PR to match the final conventions that is decided on these matters.
I'm killing this since there's no feedback to desired code style.
The small takeaway from this is:
.editorconfig needs tuning so it doesn't enforce/suggest code style that's evidently not considered desirable by all maintainers.