Fix broken LDAP connector test case syntax

[dannysauer]

I noticed that a couple of pillar test cases were passing despite an error in the test. This PR adjusts those test cases so they correctly fail upon discrepancies.

Ultimately, this ended up being a fairly significant refactor of the test code, but the test still actually works the same way; it's just a bit more concise.

[dannysauer]

The factory doesn't return the number assigned by the internal sequence (or rather it doesn't do so in a way obvious to me), so the auto increment database ID doesn't necessarily reflect the value the factory uses. This provides predictable uniqueness.

[vitoravelino]

The factory doesn't return the number assigned by the internal sequence (or rather it doesn't do so in a way obvious to me), so the auto increment database ID doesn't necessarily reflect the value the factory uses. This provides predictable uniqueness.

What do you mean by "The factory doesn't return the number assigned by the internal sequence"? Maybe an example of what was not happening from the factory usage and what is happening now with what you did?

[dannysauer]

Sure. The factory has a sequence which is used in generating the hostname and connector name. Each time the factory is used, that number increments. In these tests, we need to be able to match those fields. When this test is run stand-alone, the database auto-increment id field and sequence both start at the same point, so we can get the hostname using the database id. Because of the db cleaning, when the whole test suite runs, though, the database ID might be 5 while the sequence is 27.

We could use a static string. That would fail a unique name constraint, though, and makes debugging harder. What we want here is to ensure that the row we inserted is what we get back, which means a unique name. So this uses the test name to generate that unique ID separate from the factory sequence, saving us from relying on a relationship between the auto-increment field in the database and the factory sequence.

[vitoravelino]

You are passing the generated manually through the constructor and expecting it. Woudn't it be the same as using dex_connector_ldap.name and dex_connector_ldap.host? It's already done with dex_connector_ldap.id.

[dannysauer]

This is verifying that what we expected to store is returned. That would be verifying that was was returned is returned. :)

On September 28, 2018 2:42:07 PM wrote:

You are passing the generated manually through the constructor and expecting it. Woudn't it be the same as using dex_connector_ldap.name and dex_connector_ldap.host? It's already done with dex_connector_ldap.id. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[dannysauer]

Regarding request 1: The factory object is called north of 30 times elsewhere when the host value can be generated, so it needs to stay in there.

Regarding request 3: I added in a working destroy call, and resisted the temptation to put it in an after block. ;) I suspect that DatabaseCleaner mitigates that, but it shouldn't hurt to be explicit. That may negate the rest, since simply using a simple constant string should be fine if it's always cleaned up anyway. But if we stay with generating unique values...

Regarding request 2: That'll need some background information, based on some earlier comments. Here's basically what was done: 1) fix the expect blocks. Now the tests fail. 2) remove the delete call which didn't exist. New failure due to mismatched data. 3) add the type parameter which was causing failure. Now this group of tests pass. 4) run full test suite. These tests start failing again because the sequence is not in sync with the auto-increment ID (due to database cleanup). Figure out a way to get the example name and use that to generate a unique string to populate the name/hostname fields. 5) consolidate the duplicated variable assignment in the begin block which is necessary anyway, cleaning up the code a bit. Now tests all work.

The before block needs some more explanation. Oddly enough, you can't directly get to the example object from within an example; it is only usefully reachable inside one of the hooks like a before (the common pattern is to use a before to map example in). So, the before block is non-negotiable if the test name is to be the source of uniqueness. Because the name and host variable assignments are identical across all examples, the DRY principle Ruby is so fond of dictates combining them; the begin block is a convenient place for that.

Because the hostname and connector name calculations are simply "concatenate a string", and given the above constraint, I think leaving them in the before is actually easier to maintain than duplicating the string concatenation across two files which then have to stay in sync. I cleaned it up a little more by combining the chomp statements into one var in the exected_json, and using the simpler MD5 call (which also simplifies the comment).

[vitoravelino]

I still disagree with the necessity of having to manually generate any data without being inside of a factory.

Here's what I was suggesting with the minor changes of removing the destroy call and moving the dex_connector_ldap variable to a let! helper method:

Another improvement to make things even cleaner would be to move the custom json called within merge to inside of the expected_dex_json function. So instead of

dex: {
  connectors: [
    expected_dex_json(dex_connector_ldap, certificate).merge(
      server:    "#{dex_connector_ldap.host}:389",
      start_tls: true,
      bind:      {
        anonymous: true
      }
    )
  ]
}

we would have

dex: {
  connectors: [
    expected_dex_json(dex_connector_ldap, certificate)
  ]
}

Since we have the object the represents the model with all the generated data, we can decide what to add and what not to add inside of the expected_dex_json function.

[dannysauer]

As we go farther down the road of using the factory-generated object for data, we get closer to validating only the format of the response and not the data content. While that's fine for what this test ought to be doing, is that still a sufficient test with the other connector tests that are defined elsewhere? In other words, Is the model being fully exercised elsewhere such that it's ok to basically validate only the JSON structure here? My initial update did not make that assumption.

[vitoravelino]

As we go farther down the road of using the factory-generated object for data, we get closer to validating only the format of the response and not the data content. While that's fine for what this test ought to be doing, is that still a sufficient test with the other connector tests that are defined elsewhere? In other words, Is the model being fully exercised elsewhere such that it's ok to basically validate only the JSON structure here? My initial update did not make that assumption.

The spec changes you've submitted belongs to the PillarsController. The controller has a single endpoint and its only purpose is return a JSON. Our responsibility in this context is to test whatever affects the return of that JSON.

[dannysauer]

The spec changes you've submitted belongs to the PillarsController. The controller has a single endpoint and its only purpose is return a JSON. Our responsibility in this context is to test whatever affects the return of that JSON.

Agreed. With commit 2dba58d I've moved all of the JSON assembly logic into the expected_json definition. That reduced the content of the test examples to merely varying by a couple of constant parameters, so I pulled those out into a loop. The code has less repetition now. As a side benefit, it also now tests all four possible combinations of bind method and security negotiation rather than just two.

I also agreed with @nanoscopic's reasoning and elected to leave the destroy call in as a comment, so some future person who finds a data persistence issue might have a starting point. I think that (and the above) addresses all of the requested changes.

If we're good here, I'll squash the commits in the branch and repush.

[dannysauer]

Oh; I didn't switch to the let block because I don't see it adding anything here. The object changes across every example and is used only once in every example, so the lazy evaluation doesn't save anything. The example is so compact that, IMHO, the parens and colons and braces in let add more distracting visual clutter than simply using an = for a local var.