[#64589182] Ignoring Id parameter when diffing Nat and Firewall rules. #25

merged 18 commits into from Mar 18, 2014

3 participants


The automatically generated Id parameter would cause an issue with Diff display when inserting new Firewall or Nat rules - every rule following the insertion would have its Id incremented, and hence be 'different' (despite the content remaining the same).

This PR adds two new Differ classes: FirewallConfigurationDiffer and NatConfigurationDiffer

These strip out the Id field (in FirewallRule and NatRule sections) before comparison, and hence provide clean diff output.

mikepea added some commits Mar 6, 2014
@mikepea mikepea Fix indentation in nat_service generator 0c16404
@mikepea mikepea DRY out differ tests before adding more classes
In anticipation of adding the Nat and Firewall differ classes
(to handle Id fields), DRY out the ConfigurationDiffer and
LoadBalancerConfigurationDiffer tests to have a common set
of test cases that we can use to ensure all differ classes are
obeying the same basic set of rules.
@mikepea mikepea Add NatConfigurationDiffer class to ignore Id
When new nat_rules are inserted, the resulting generator output
dutifully increments the Id value of NatRules that follow.

This creates a problem when comparing with ConfigurationDiffer,
since the rules after our insertion have not changed, bar their
Id value increasing in relation to the new rules.

This commit adds a NatConfigurationDiffer class which strips out
these Id values before comparison.
@mikepea mikepea Add missing newline from coverage message 7f2a309
@mikepea mikepea Add FirewallConfigurationDiffer class to ignore Id
(as per the NatConfigurationDiffer class in daebc34)

When new firewall_rules are inserted, the resulting generator output
dutifully increments the Id value of FirewallRules that follow.

This creates a problem when comparing with ConfigurationDiffer,
since the rules after our insertion have not changed, bar their
Id value increasing in relation to the new rules.

This commit adds a FirewallConfigurationDiffer class which strips out
these Id values before comparison.
@mikepea mikepea DRY out the ConfigurationDiffer classes
Make ConfigurationDiffer the parent of all per-service differs,
since they are all sharing common behaviour, just altering
what they consider to be 'the configuration for comparison'
GDS Operations member

I'm not very happy with the use of inheritance or the abstracting of the test away from where the tests are being run... am thinking about whether to change it or merge it as is.

GDS Operations member

I'm going to make some changes to this.

annashipman added some commits Mar 14, 2014
@annashipman annashipman Do inheritance in a clearer way
There's no need for code to be repeated in every subclass. The only thing that really needs to change is what the unused field is.

The difference between this and what was there before is that the unused field is stripped from both local and remote whereas in the case of the Load Balancer, it won't be in the local. This doesn't matter though, as there is a test for whether it's present before attemptint to remove it.
@annashipman annashipman Artefacts of the way inheritance was being done 4119af6
@annashipman annashipman Simplify method calls to what is required 26b7269
@annashipman annashipman Remove common differ cases from subclass tests
The looping through test cases is bad (this will be explained more fully in the next commit). Removing them from the subclasses. They will later be reimplemented.
@annashipman annashipman Looped through tests are bad
Instead of failing, they just don't run if the circumstances aren't right. For example:
 - 0f29d67: one of tests wasn't running due to a very small typo
 - gds-operations/vcloud-core@ebd3305: the code they were testing is no longer present, so the tests just don't run; when I remove code that is tested, I expect the tests to fail.

This makes them fairly useless as tests. This commit reimplements the tests as actual tests. They only test the configuration differ file as this stage.
@annashipman annashipman Rewrite config differ subclass tests as tests
Rather than as loops, as per 9883ea5.
@annashipman annashipman Make the config differ tests shared_examples
This will allow the subclasses to call the tests of the superclass without repeating all the tests. Also changed the name of the file to include to reflect what it now is.
@annashipman annashipman Pass the type of Configuration Differ
So that subclasses are testing themselves rather than all just testing ConfigurationDiffer again. `?w=1` will help to see the changes in this commit as I also fixed indenting.
@annashipman annashipman Make subclasses run the Config Differ tests bc8f32a
GDS Operations member

Hi @danielabel - I've rewritten this PR and it would be good if you could take a look. I know there are a lot of commits but hopefully it should be fairly straightforward.

I've done two things:

  1. Improved the inheritance of the Configuration Differs so they only implement the one thing that has to be different (ie which field to suppress when doing diffs).

  2. Reimplemented the tests, moving them away from the looping through a constant of test cases in another file (which is bad 9883ea5) and instead using shared_examples https://www.relishapp.com/rspec/rspec-core/v/2-11/docs/example-groups/shared-examples so the subclasses just say it_behaves_like "a configuration differ" to run all the configuration differ tests against that class.

@danielabel danielabel and 1 other commented on an outdated diff Mar 17, 2014
@@ -20,7 +20,7 @@
# do not change the coverage percentage, instead add more unit tests to fix coverage failures.
if SimpleCov.result.covered_percent < 60
- print "Coverage is less than acceptable limit(71%). Please add more tests to improve the coverage"
+ print "Coverage is less than acceptable limit(71%). Please add more tests to improve the coverage\n"
danielabel Mar 17, 2014

Not your fault, but it looks like the coverage got reduced on line 21 and it's not reflected in the copy - fix in this or do we need a pivotal ticket.

annashipman Mar 18, 2014 GDS Operations member

I can go one better. See 1616c2d

@annashipman annashipman I've added a lot of tests here
So coverage is higher. I'm not entirely convinced by this, because the tests were all in one area - it's clearly not testing lines of code or paths through the code. However, this is the tool we are using for coverage at the moment so might as well make use of it.
@danielabel danielabel and 1 other commented on an outdated diff Mar 18, 2014
@@ -8,7 +8,19 @@ def initialize local, remote
def diff
- ( @local == @remote ) ? [] : HashDiff.diff(@local, @remote)
+ ( stripped_local_config == stripped_remote_config ) ? [] : HashDiff.diff(stripped_local_config, stripped_remote_config)
+ end
+ def stripped_local_config
+ strip_unused_field_from_config(@local) unless @local.nil?
+ end
+ def stripped_remote_config
+ strip_unused_field_from_config(@remote) unless @remote.nil?
+ end
+ def strip_unused_field_from_config(config)
danielabel Mar 18, 2014

This method name threw us for 5 mins. We think that what it does is remove fields to be ignored for/excluded by the differ. Is that correct and is there a clearer method name that could be used?

annashipman Mar 18, 2014 GDS Operations member

Very good point. How about remove_fields_for_differ_to_ignore? And to that end, do you think stripped_local_config and stripped_local_config are clear enough? Perhaps if I call the method strip_fields_for_differ_to_ignore?

danielabel Mar 18, 2014

We like both of your suggestions 👍

annashipman Mar 18, 2014 GDS Operations member

And it is so! cf9c995


general thoughts: we think this code is testing the ConfigurationDiffer three times, due to the nature of it using inheritance - needing to test once for each instance. It's good that we don't have those tests verbatim, or in a loop.

We considered the alternative of having a concrete differ that has exclude/ignore filter behaviour injected into it (composition style). We can see how that would be better if this were to scale any further, and might make the concerns in the tests clearer also.

We weren't sure if you had considered and dismissed this option, or if as the code is unlikely to scale past 3 instances it wasn't valued enough to re-implement in this way?

/cc @zoodor

GDS Operations member

Ha! Yes, I completely agree. I told Mike I wasn't a fan of inheritance and preferred composition, but he did it this way. I continued with inheritance because basically I am rewriting someone else's piece of work as he is now on paternity leave - arguably though I've made so many changes to his original work that I might as well have changed it entirely.

My feeling on this is that at this stage I don't want to put any extra time into this because of the points you make.

It's not exactly testing the ConfigurationDiffer three times though - it's testing that the subclass does the same things required of the superclass - a check worth putting in when inheritance is about...


Super! 🌼

@danielabel danielabel merged commit 415ab46 into master Mar 18, 2014
@danielabel danielabel deleted the 64589182_id_param_diff branch Mar 18, 2014
@annashipman annashipman added a commit that referenced this pull request Mar 18, 2014
@annashipman annashipman Reduce coverage required
#25 increased the coverage required dramatically from 60% to 95%, which is fine when running unit tests, but when integration tests are run, it's not enough.

I am not convinced of the value of this coverage tool, so I am working around this problem by reducing the coverage I claim to have.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment