11 ms·
I_suck_and_my_tests_are_order_dependent
- Groxx 4y agoRelated: Go finally added support for randomizing test order in 1.17! https://go.dev/doc/go1.17 https://go.dev/doc/go1.17 it's disabled by default. tl;dr: use `-shuffle on` so your tests don't suck (as much)
- onionisafruit 4y agohttps://github.com/rails/rails/commit/e81f3c210eca074ed6227bd1c40835d44761c09e https://github.com/rails/rails/commit/e81f3c210eca074ed6227b...
- drewcoo 4y agoSad. It's meaningless without the shaming.
- ryan-allen 4y agoShaming others is and has been part of the Ruby on Rails culture since its inception.
- jeroenhd 4y agoI sort of agree with the idea, the developer calling this code may not be responsible for the mess that necessitated the call in the first place. A name like this_code_sucks_and_these_tests_wre_order_dependent would be much more appropriate. It'd still be a name that would at least raise some eyebrows during code review so you'd have to write some kind of explanation why you're enabling this.
- abledon 4y agowhich commit originally put it in? That guy was a comedian
- jan_Inkepa 4y agoThis is the commit that added it, with the description "users_dont_suck_but_only_we_suck_and_only_our_tests_are_order_dependent!" https://github.com/rails/rails/blob/6ffb29d24e05abbd9ffe3ea974140d6c70221807/actionmailer/test/abstract_unit.rb https://github.com/rails/rails/blob/6ffb29d24e05abbd9ffe3ea9... So the "I" refers not to the general user of the function, but to the implementer of the functionality, it seems. Code philology of the day!
- trynewideas 4y agoYou didn't follow the blame further. First, the flag is provided by minitest. That commit is an implementation of the flag in Rails. Second, 6ffb29d moved it to prevent Rails's test framework from setting it _by default_. 281f488[1] actually added it to Rails. minitest/minitest#a4553e2[2] appears to have added the docstring and test case to minitest. 1: https://github.com/rails/rails/commit/281f488fffc176084bf77c3ba5b1f1c4d6ab5680 https://github.com/rails/rails/commit/281f488fffc176084bf77c... 2: https://github.com/minitest/minitest/commit/a4553e2e127072c93edfaf9eabc1d8eb187d437f https://github.com/minitest/minitest/commit/a4553e2e127072c9...
- abledon 4y agodid you find all this through _clicking_ in the github UI or did you use a fancier tool on your computer? impressive sleuthing!
- trynewideas 4y agoAll through GitHub. 1. From https://github.com/rails/rails/blob/6ffb29d24e05abbd9ffe3ea974140d6c70221807/actionmailer/test/abstract_unit.rb https://github.com/rails/rails/blob/6ffb29d24e05abbd9ffe3ea9..., click "Blame" on the header bar over the file contents. 2. Scroll down to the line and click on the commit in the left column. 3. Scroll down to the part of the commit that removed the line from its previous location, in activesupport/lib/active_support/test_case.rb. 4. Click the three-dots menu in that file's header bar and select "View file". 5. Click "History" in the header bar of the contributors, above the file contents. 6. I guessed here that it was introduced in commit 281f488 based on its message: "Use the method provided by minitest to make tests order dependent". Clicking through to it confirms that. There's also a comment here that identified the problem which led to, and provided context for, the change in 6ffb29d. The OP is from minitest's documentation, so to find the introduction in minitest, it's basically the same process. 1. Go to https://github.com/minitest/minitest https://github.com/minitest/minitest. 2. Search the repo for the method name. Even just "i_suck" will match the commit. 3. Select the oldest commit in the results. That's a4553e2.
- sublinear 4y agoHow incredibly arrogant. 99% of the time that happens because the code sucks and the tests are impossible to write any other way without significant effort to mock resources and/or just refactor the code (which of course breaks the old tests). Anyone know if this is one of the many papercuts that probably killed off ruby (poor dev culture)? I never followed the language much.
- kissgyorgy 4y agoIt seems like you need to call this function :) and not surprisingly, your comment is totally incorrect, tests can be implemented independent of each other regardless of the quality of the code they are testing. This doesn't mean they can run in parallel, but every test should be running independently from each other no matter what.
- sublinear 4y agoOh boy. The fact that tests test arbitrary things should be enough for you to reconsider what you're saying. You can't possibly know if I'm "totally incorrect". I'm not talking about sharing state between tests. I'm talking about the code being tested having global state that is deeply entrenched in how it works, it's unreasonable to refactor, yet a test suite must be written. What truly sucks is someone not even writing tests at all for the tangled parts which is extremely common to find because it breaks someone's precious mental model and they don't want to put their name on it.
- imran-iq 4y ago> I'm talking about the code being tested having global state that is deeply entrenched in how it works Could you give a specific/actual example of this? Technically a database is global state that is deeply entrenched in how an app works, but its trivial to make order independent tests that require certain records to exist.
- toolz 4y agoThe way I see it, if your tests are dependent on mutable global state, they are bad. If they are not dependent on mutable global state, then other tests can't break your tests. I'm not confident enough to say there aren't exceptions here though.
- KMag 4y agoA couple of related practices I've picked up over the years: (1) using long names when implementing things that are necessary but discouraged (2) putting dange_danger in names of things that should never be used in production. The former introduces just a tiny bit of friction, but it's often enough to encourage developers to use the preferred API calls. The latter I learned almost 20 years ago from Google's "Mustang" search back-end. If running a test instance of Mustang without bringing up a ton of dependency processes, there was a --danger_danger_must_not_fail true commandline flag (or something very similar) to turn a bunch start-up sanity check aborts into warning log messages. If any code reviewer sees "danger_danger" being added to production command line flags, they'll almost certainly ask the right questions.
- suzzer99 4y agoThis is the flag required by our script to create and merge pull requests from one environment to another: MERGE_APP_REPOS_YES_I_ALREADY_MERGED_LIB_REPOS_AND_WAITED_FOR_THEM_TO_FINISH_BUILDING
- craigmcnamara 4y agoThis seems like a broken process to me.
- lamontcg 4y agoSeems like that is intended to never be used by humans and is used by a process that has always done the other thing first (unless you're manually debugging the process itself in which case you'll copypasta the silly long thing).
- suzzer99 4y agoIt's a script run by a human when they want to push the changes in one environment to another. Our various apps consist of 3-10 repos. All the apps are backed by two shared lib repos (one for node lambdas, and one for C# lambdas). Merging each repo manually to do an environment push is cumbersome. The script creates and merges pull requests on github for the lib repos and app repos - from one environment (branch) to another. First you have to do the lib repos, and if there are any changes wait for the CICD jobs to finish. If there are no changes to the libs, the script indicates that no pull requests were needed. I have no idea how to get a local bash script to poll for when AWS CICD jobs are done, nor do I really want to spend days on that. So I just added that parameter to make sure I (and anyone following me) runs the lib merges first and waits for them to finish building before merging the app repos. The job without any flag just merges the lib repos.
- kyleyeats 4y agoFlagellants in tech are weird. You had to use a bad pattern once. You'll be fine.
- 6LLvveMx2koXfwn 4y agoWe run ordered Integration Tests to save time as the set-up is onerous. Is that bad?
- readthenotes1 4y agoIt's bad, no doubt. The question is What are the costs of the alternatives?
- daniel-s 4y agoI think that your ordered tests are the setup.
- teeray 4y agoLike everything in tech, it’s a tradeoff. You’re trading the potential for side-effects from one test to influence the behavior of subsequent tests without you knowing for decreased setup time. This might be a good thing or a bad thing—it depends! Engineering is balancing these tradeoffs and mitigating the risks their downsides bring.
- evil-olive 4y agonot necessarily - the key is that you want to make the ordering dependencies explicit. the real issue comes from tests that have accidental or implicit order dependencies, such that adding a test to the middle of a file might cause subsequent tests to fail because they expected something from the previous test. or, refactoring a large file into two smaller files might result in a bunch of test failures, because the tests happened to depend on system state specific to the original larger file. implicit ordering dependencies also tends to assume single-threaded test execution. a common strategy to speed tests up is to run them in parallel, and that can cause havoc if the tests aren't carefully scoped to act only on resources created by that test (such as, a test of user creation that in its teardown phase says "eh, just delete all users with a username starting with "test_")
- zamalek 4y agoNow you'll struggle to run them completely in parallel (in complete isolation, i.e. unique database server and what-have-you). This will probably matter if you want to run them in CI. It's a trade-off though, and itests are a nasty monster to deal with no matter what.
- tomlockwood 4y agoI've noticed a lot of developers that have this kind of code purity complex where their way of doing things is superior even if the actual code Works - which I think is the number one most important thing about code. The code purity complex is a self-limiting decision. Those developers will be sitting at home fuming, I guess, as I cheerfully build a new feature on VB6 code last changed in 1999 that relies on using "On Error resume next", on an obscene contract rate. Working only with Good code is a luxury decision, and writing Code That Sucks isn't necessarily bad, if thats the fastest/cheapest way to write code that works. The end user almost as a rule, doesn't care about your code purity. A developer probably couldn't even explain that purity in a way that would make the end user understand its value. This method name indicates a lack of understanding that finding the right balance of speed, cost and quality is a compromise. Yes, tests that can run in isolation are "better".
- culi 4y agoEither you mostly worked by yourself or on a small team or you don't realize how much your coworkers hate you. In my experience, code quality isn't usually about some purity complex. It's a recognition of the fact this code may have been written once but will have to be read many more times
- revscat 4y agoI work with someone like this. And yes, we hate him. Unfortunately he’s the CTO. Of a YC-backed company, even!
- tomlockwood 4y agoActually the most common reason I get a job is referrals from ex-coworkers. ed: Also probably worth noting the reason for this may partially be because I don't say their code "sucks".
- djur 4y agoIf you don't have a lot of ego about code purity you probably won't mind typing a mildly self-deprecatory thing in your test setup, either.
- ivmaykov 4y agoI wrote an unsafe library at a FAANG and used similar naming conventions. The init function was named along the lines of “MyClassName_UNSAFE::initUnsafeIUnderstandTheRisks()” And the library itself was called something like “library-name-unsafe-my-team-name-must-approve-diff” So anyone trying to use it would have to add a library with that name to their list of dependencies and would come to us asking for a code review, and more than half the time we would redirect them to a safer alternative (valid use cases for the unsafe library were few and far between).
- WirelessGigabit 4y agohttps://reactjs.org/docs/react-component.html#unsafe_componentwillmount https://reactjs.org/docs/react-component.html#unsafe_compone... This you?
- ivmaykov 4y agoNo, mine was in closed-source internal code.
- cwillu 4y agoYou're a bad programmer. I'm a good test runner.
- zemo 4y agohave fun testing a stateful system without order-dependent tests
- andreareina 4y agoSet the state to a known state, test, assert the resultant state is what I think it should be. Also testing a sequence of operations is fine, but that's one test not n of them. This is analogous to integration tests: they test several units but it's a single test.
- zemo 4y ago> Set the state to a known state how do you guarantee that the state that you set as the initial state is reachable in your system?
- andreareina 4y agoWith an integration test. It executes multiple operations that are order dependent, but then this is a single test, not multiple.
- speed_spread 4y agoState, not even once.
- Jtsummers 4y agoCan each test be executed separately? Then it's a collection of tests. Otherwise, that's just one test with many steps posing as separate tests.
- bdcravens 4y agoWouldn't you be testing state transitions within the context of a single test however? The system having order dependency isn't the same as the tests having order dependency. In other words, tests shouldn't be coupled together. Reinitialize, and rerun steps, between each test that stands alone.
- 4y ago
- michaelteter 4y agoMake your models thin. Then make your models simple data structures. Then your tests can build a data structure with just what is necessary for the test. Then your tests don't need to be ordered. The common problem is that models are fat, and to test them requires building up some very complex scenario. Every step mutates something. So the step order is critically important. This is absolutely not necessary if your functions are pure. It's also easier if your models are simple data structures, so they require less setup. edit;added - if you make your models thin, then you need to add "service" modules which encapsulate the business logic. Taking it outside the models makes it more testable with less effort, and it also makes it less dependent on your framework.
- otikik 4y agoHow do you make all functions pure, though? At the end of the day, what makes a computer interesting is that it is stateful.
- michaelteter 4y agoOh come on. The edge functions do mutations, but the interior functions are just functions. Somehow mathematics manages to get things done in this manner.
- otikik 4y agoCan’t tell wether you are being ironic or not.
- michaelteter 4y agoNo, I am not. Perhaps I am not communicating my meaning well though... A model (class) can represent a certain kind of data, and an instance of that model (object) can represent the details of one instance of that kind of data. On this I imagine we can all agree. OOP attempted to organize systems by bringing the data together with the operations that could be performed on it (class/instance methods). And with the goal of hiding the implementation details, the instance methods would work on Self, mutating the data members of a given object. There are many problems with this approach, and one is this: You often have business rules which involve greater than one model, such as how a shopping cart checkout will touch a payment record, a cart, product inventory, etc. The tidy models suddenly get wrapped up in a bigger system where some operation needs to carry and pass around multiple objects and push each object to mutate itself in various ways (or worse, just reach in and directly mutate the objects from outside). So in a real system (complex), you can find yourself at a point where some object has changed, but it's not clear why or where. And adequately writing tests for a system like this is very difficult, because the combinations of possible scenarios is almost beyond reason. Now imagine an alternate approach where you can still have a data model, but the operations you might perform are separate and do not mutate anything; instead, they take data (some representation of the model, whether a thin OOP object or a simple data structure such as a hash), they perform whatever operation they need to perform, and they return a new representation of that data (where presumably the details are different from the original data that was passed in). Writing tests for this is very easy now, since the only setup required is whatever is minimally necessary for that small (single responsibility) operation. And it is more observable, because now a series of functions which make up a bigger operation have a history of outputs which can be compared or saved. I have demonstrated this approach in a real world situation where my team built a greenfield system to replace a legacy system. Both were Rails, and the original used the typical fat model Rails approach as you would learn from reading Rails books and guides. The new replacement had minimally thin models and "service" modules which had the logic. Test coverage in the replacement system was much higher, but lines of code were much lower. Also tests ran multiple times faster in the new system. Code was easier to reason about, and debugging was easier. The only downside, as such, was that there were now many smaller functions. So choosing good function names took more work and typically resulted in longer names (to express the intent of the function). There was initial resistance to this approach, but that resistance later became promotion of the approach to other teams. Ultimately this approach takes most of the business logic out of the Rails framework, which also makes it easier to transition to separate service processes/systems when things grow too big. Really it's not so unlike the intention and benefits of separating data from actions from display as the model-view-controller paradigm does.
- jay-barronville 4y agoFunny. At a previous employer, we had a few services that needed access to some data we hadn’t put behind a service yet. For bureaucratic reasons I won’t bore y’all with, our team wasn’t given the green light to create the needed service to provide access to the data, so a coworker and I created an endpoint on an existing service but we added a requirement that an oddly specific and undocumented HTTP header calling out management must be provided in every request made to the endpoint and then we waited. At some point, someone else needed to use the endpoint and came to us asking why it wasn’t working. We told him about the header. He was annoyed and complained to his manager. The very next week, we got what we wanted: Our new service was up and running! We also got into a bit of trouble over this, but that’s just an extraneous detail — haha.
- otikik 4y agoIf you had done nothing that service would not exist. Ask for forgiveness, not for permission.
- andrew_ 4y agoPNPM has an option called `shamefully-hoist` and it's by far my favorite option of all time: https://pnpm.io/npmrc#shamefully-hoist https://pnpm.io/npmrc#shamefully-hoist
- benatkin 4y agoHmm, 11 characters are reasonable for this sort of a thing. Pytest's disable_test_id_escaping_and_forfeit_all_rights_to_community_support are too many characters IMO.
- gweinberg 4y agoMySQL Command line has a flag called i-am-a-dummy, I know it's supposed to be funny but I think it's actually a good idea to run in dummy mode when you can, so the insulting name might be counterproductive. I think most people who have used SQL have updated with an insufficiently restrictive where clause, or even forgotten to include the where clause entirely, at least once.
- jammycrisp 4y agoPytest has an equally deprecating option for a different "use case": disable_test_id_escaping_and_forfeit_all_rights_to_community_support = True https://docs.pytest.org/en/6.2.x/parametrize.html#pytest-mark-parametrize-parametrizing-test-functions https://docs.pytest.org/en/6.2.x/parametrize.html#pytest-mar...
- kelnos 4y agoI'd say not equally. The pytest one is basically "we get that your use case might be a little off our beaten path, and we want to allow you to be successful, but we aren't prepared to take on the support burden of it". The minitest thing is "if you don't do things how we envision with our limited perspective, you are bad at programming".
- Dylan16807 4y agoI think pytest understands that they only need the escaping because their own code is subtly broken in many places. And look how much it screws up text: https://user-images.githubusercontent.com/1457682/57952631-be4ed180-7928-11e9-82c6-6f43eeef03ee.png https://user-images.githubusercontent.com/1457682/57952631-b...
- djur 4y agoThere is no reason to have order-dependent unit tests. It's good for test tools to do everything they can to keep you from having them.
- Bjartr 4y agoI have a method in our Java app`iKnowWhatImDoingGiveMeTheSession()` that will only work if you have set the system property `doYouReally=Yes, I really know what I'm doing` It should never be used in production, but can be helpful to have available when doing certain kinds of investigation.
- deleted 4y ago[deleted]
- thehappypm 4y agoWhat you’re describing is a debugger!
- febusravenga 4y agoAll my migration/tweak-production-db scripts run by default in dry run mode and require `--really` to override it. It's good excercise to write mutating code as much transactional way as possible.
- Bjartr 4y agoA debugger doesn't remember the proper steps for me, a written function does. I absolutely use this function in a debugging context. If it didn't exist, could I get at what I needed? Yes, but it's inconvenient to use reflection to get at private Java fields during a debugging session. Not impossible, just inconvenient. This function exists as a convenience, but it has to look weird, otherwise its convenience might lure someone into using it to do the wrong thing for a production use case. Good APIs make the right thing easy and the wrong thing hard.
- KETpXDDzR 4y agoIs this still ok in today's over-sensitive environment where "blacklist" files are taboo. (Btw, "blacklist" comes from British chimney cleaners).
- jdwithit 4y agoContext changes. There are various homophobic slurs that used to mean something else but now have a new, extremely hurtful meaning. And nobody tries to use the previous definition except in bad faith to be a pot-stirring asshole. By the same token, nobody is talking about British chimney sweeps when they say blacklist. Unlike the first example, I don't think people are trying to be hurtful by using blacklist. But it's not hard to connect the dots of "black = bad, white = good" and see why people dislike it. It costs you nothing to switch terms to blocklist/allowlist. Or change your git default branch from master to main. Thinking about the feelings of someone other than yourself isn't being over-sensitive. It's being human.
- Dylan16807 4y agoThe thing is, blacklist and whitelist have not gained any new meanings any time recently. (And I can't find anything about this supposed chimney meaning?) It was always about the actual colors.
- droopyEyelids 4y agoAt the company I work for, there is an internal effort to rename "Black Friday" to "Retail Friday". Coming up with reasonable explanations to justify this stuff is starting to feel like when I was a child in religious education & would rack my brain to come up with my own explanations for the contradictory and implausible aspects of what I was being taught.
- thiht 4y agoI can't wait for the word "black" to be banned in favor of "dark gray"
- KETpXDDzR 4y ago
- benatkin 4y agono mames
- Hirrolot 4y agoI disagree that ordered tests are bad. See, I don't even have mutable state in my program -- but still have ordered tests for another reason: the ease of debugging. Say, if module A depends on module B, then B should be tested first, only then A, since if you test in the other direction, you might have a hard time figuring out whether A or B is misbehaving.
- frogulis 4y agoDoesn't that only apply if you stop the entire suite upon receiving one failed test?
- Hirrolot 4y agoEven if I can have multiple failing tests, unordered testing might get tricky. For example, as I said, A depends on B, and suppose that both A and B tests are failing. If I (or really anyone else in the team) forget that A depends on B, they might be tempted to debug the A test first, only later figuring out that this is B that needs to be fixed.
- Jtsummers 4y agoThis doesn't make sense to me. If you know that A depends on B and you run your test suite while B is broken, then both A Suite and B Suite will fail. But the order is irrelevant, you'll know that B (the one A depends on) is broken and that should be the focus of your investigation. Why would there be any confusion if the tests executed in a random order?
- Hirrolot 4y agoI've answered it in the other comment -- because I might not know the entire module dependency graph. For example, if the code is not mine.
- Jtsummers 4y agoIf the code isn't yours then you have to hope they ordered the tests correctly or, you know, figure out the dependency order. Which isn't hard: - Use a static analysis tool that kicks out a dependency diagram. That literally shows the dependency order. - Look for import/include/using statements. That shows the dependency order. - Look for inherits/implements part of class definitions. That shows the dependency order. - Look at the build commands. It'll tell you the dependency order. - When you see a function, constructor, whatever being used, you have a dependency. And if the code is yours, and you don't know the dependency order, then fix that. Write it down or something.
- Luddberg 4y agoOr use test_order :alpha