Most active commenters
  • stickfigure(3)

←back to thread

294 points NotPractical | 12 comments | | HN request time: 0.612s | source | bottom
Show context
xnorswap ◴[] No.41856752[source]
> Redbox.HAL.Configuration

> .ConfigurationFileService implements IConfigurationFileService

> STOP MAKING SERVICES AND FACTORIES AND INTERFACES AND JUST READ THE FUCKING

> JSON FILE YOU ENTERPRISE FUCKERS

I know it's cool to "hate" on OO, but "just read the fucking file" doesn't work if you want to run your unit tests without reading a fucking file.

It makes sense to abstract configuration behind an interface so you can easily mock it our or implement it differently for unit testing.

Perhaps you also want to have some services configured through a database instead.

This isn't a ConfigurationFileServiceFactoryFactory.

replies(12): >>41856822 #>>41856831 #>>41856836 #>>41856965 #>>41857895 #>>41858054 #>>41859117 #>>41859509 #>>41859750 #>>41859882 #>>41860221 #>>41864182 #
1. rcxdude ◴[] No.41858054[source]
But why is it so hard to read a file during a unit test? Files are pretty easy to mock in many different ways, all of which are pretty fast. You don't need a special-purpose interface to be able to test the code that uses a config file.
replies(3): >>41858083 #>>41858297 #>>41858623 #
2. wobblyasp ◴[] No.41858083[source]
It's not, but maybe I don't want to create a file for the tests. The point theyre trying to make is that it's a personal preference and not an obvious "this way is better"
3. stickfigure ◴[] No.41858297[source]
Let's say you want to test bootstrapping your system with various configurations.

You could make a few dozen different configuration files. Or maybe it's more than that because you want to test permutations. Now you're maintaining a bestiary.

So instead you think "I'll write code that generates the config file for each test". And that's reasonable sometimes.

On the other hand, the single-responsibility principle can be reasonably applied here and "reading the config data" is a good single responsibility. You don't want to have to write code to muck with json or xml every time some component needs a configuration value. So there should already be a clean boundary here and it often makes sense to test the components separately.

There's not one rule. The article author sounds like an excitable jr engineer that's never written software at scale.

replies(3): >>41859388 #>>41860580 #>>41861658 #
4. xnorswap ◴[] No.41858623[source]
Perhaps a better example is a real world example I ran into just this week.

I found out that our unit test suite would only pass when run under elevated credentials. Our internal developer tooling had been running under semi-privileged credentials for years, and was the usual way of triggering a full unit test suite run, so no-one really noticed that it didn't work when run at a lower elevation.

When run from a lower privilege, a unit test was failing because it was failing to write to a registry key. I first double checked that I wasn't accidentally triggering integration tests, or that the test should be tagged integration.

But no, we had simply failed to abstract away our registry writes within that service. Of course no unit test should be writing to the real registry, but this settings manager was just being new'ed up as a concrete class, and there was no interface for it, and so it was just naively making registry edits.

This settings class wrote directly to the windows registry as it's data-store wasn't noticed as an issue for years, because all the times it had previously been run, it had been under credentials which could access that registry key.

And yes, there are different ways we could have mocked it, but favouring a concrete class meant this registry edit was happening unnoticed across all our unit test runs. And I suspect this might have been behind some of the dreaded flaky test syndrome, "I just tweaked a CSS file, why did my PR build fail?". Because 99% of times it was fast enough that it didn't cause issues, but with just the right concurrency of test execution, and you'd have a problem that wouldn't show up in any meaningful error message, just a failed test "for no reason".

Why shouldn't unit tests read real-world files? Because that introduces brittleness, and an inherent link between tests. If you want fast and easy to parallelize tests they need to have no real-world effects.

A test suite which is effectively pure can be executed orders of magnitude more quickly, and more reliably, than one which depends on:

  - Files

  - DateTime.Now (Anywhere in your code. A DateTimeFactory which you can mock might sound ridiculous, but it's perhaps the best thing you can do for your code if your current code / tests run on real dateTimes. Even for production code, having a DateTimeFactory can be really helpful for relieving some timing issues. )

  - Databases ( This is more "obvious", but still needs to be said! )
And so on. A unit test suite should boil down to essentially pure statements. Given inputs A,B,C, when applying functions f, g, h, then we expect results h(g(f(A,B,C))) to be X.

This can also be the difference between a test taking <1ms and taking <10ms.

As a final point, you're usually not wanting to "test the code that uses a config file", you want to test code which you don't care if it uses a config file.

The "Code that uses a config file" should be your Configurator class. What you actually want to test is some service which actually produces useful output, and that contains business logic.

Yes, "separation of concerns" can be taken too far, but having something else responsible for the manner in which your service is configured so that your service can just take in a business-domain relevant typed settings object is helpful.

As I've said elsewhere, config is actually a terrible example, because it's essentially a solved problem, MS released System.Configuration.ConfigurationManager ( https://www.nuget.org/packages/system.configuration.configur... ), and you should probably use it.

If you're not using that, you ought to have a good excuse. "Legacy" is the usual one of course.

replies(2): >>41860108 #>>41861574 #
5. dogleash ◴[] No.41859388[source]
> that's never written software at scale.

Is this like a never version of that insult where people would say someone's opinion doesn't matter because they worked on a project that never shipped (regardless of how much or how little they contributed to the failure)? Just replacing it with an AWS bill-measuring contest?

replies(1): >>41861108 #
6. shagie ◴[] No.41860108[source]
> DateTime.Now

I've got this in several places.

I have code that needs to check if other date is within two weeks of today. I have test data.

I could either modify the test data based on today's date (adding other logic to tests that itself could be faulty), do something while loading the test data... or have the date to be used for comparisons be injected in.

That date is July 1, 2018. It was selected so that I didn't have difficulty with reasoning about the test data and "is this a leap year?" or across a year boundary on January 1.

Its not a "I don't trust it to work across those conditions" but rather a "it is easier to reason about what is 60 days before or after July 1 than 60 days before or after January 1.

And returning to the point - injectable dates for "now" are very useful. Repeatable and reasonable tests save time.

7. pphysch ◴[] No.41860580[source]
> Now you're maintaining a bestiary.

Any battle-hardened test suite is already a bestiary. Having a subfolder of diverse & exemplary config files, that could be iterated over, is not adding much to the pile.

replies(1): >>41861608 #
8. stickfigure ◴[] No.41861108{3}[source]
"Software at scale" is different from "data at scale" is different from "compute at scale".

But yeah, when I hear "STOP MAKING SERVICES AND FACTORIES AND INTERFACES AND JUST READ THE FUCKING JSON FILE YOU ENTERPRISE FUCKERS" I think "developer who's never worked on anything more complicated than a chat app, and isn't old enough to have learned humility yet".

replies(1): >>41862509 #
9. ◴[] No.41861574[source]
10. stickfigure ◴[] No.41861608{3}[source]
A totally reasonable approach! Sometimes.
11. nucleardog ◴[] No.41861658[source]
The interface in question is `IConfigurationFileService`. I can only guess at the actual interface, but based on the name it doesn't sound like it's abstracting away the need to put your configuration into files.

Could just be a case of bad naming and it solves everything you're saying. But it sounds like pointless enterprise-y fuckery to me.

I would not say the same thing about `ConfigurationFileService : IConfigurationLoader` or something.

12. ◴[] No.41862509{4}[source]