[YAML] Added unit tests to Dumper #7144
Conversation
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | [] |
| License | MIT |
| Doc PR | no |
| { | ||
| $this->dumper->setIndentation('8'); | ||
| $this->assertTrue(is_int($this->dumper->getIndentation())); | ||
| $this->assertEquals(8, $this->dumper->getIndentation()); |
There was a problem hiding this comment.
You could do it with a single assertSame assertion.
There was a problem hiding this comment.
Ah yes, I will update.
|
I don't like the fact that you are adding a getter for the only purpose of reaching 100% coverage (which could be achieved differently by checking that the dumper can indeed use 8 spaces when dumping) |
|
Ok, fair point, I will amend. |
|
I also thought of using reflection for the private property, as checking 8 space dump is less of a unit test as using multiple methods, thoughts? |
|
Another way to look at it, is if the property has a 'setter' why should it not have a 'getter' too? i.e. If the developer can 'set' it, why cant they 'get' it too. Just another thought, once the best way to move forward is confirmed, I will update my other tests accordingly & submit them. |
|
Another solution would be, to extend the @stof What do you think? |
|
@Baachi IMO, the unit test should ensure that we can actually change the indentation of the dumped code (which is what we want to do). We don't bother about being able to get the indentation (we don't even have a method for it currently), we want it to be applied. The Dumper is not a configuration object. It is an object doing some work. So testing that a new getter returns the value will not ensure that changing the indentation is working. |
|
@stof Ah yes, I understand you. So my solution is wrong, @eddiejaoude should call |
|
Ok, thanks for the clarification guys. I will get on the case shortly! |
…d indentation by using dump
This PR was submitted for the master branch but it was merged into the 2.1 branch instead (closes #7144). Commits ------- 448983c [YAML] Added unit tests to Dumper Discussion ---------- [YAML] Added unit tests to Dumper | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | [] | License | MIT | Doc PR | no --------------------------------------------------------------------------- by stof at 2013-02-21T11:28:55Z I don't like the fact that you are adding a getter for the only purpose of reaching 100% coverage (which could be achieved differently by checking that the dumper can indeed use 8 spaces when dumping) --------------------------------------------------------------------------- by eddiejaoude at 2013-02-21T11:33:03Z Ok, fair point, I will amend. --------------------------------------------------------------------------- by eddiejaoude at 2013-02-21T11:35:14Z I also thought of using reflection for the private property, as checking 8 space dump is less of a unit test as using multiple methods, thoughts? --------------------------------------------------------------------------- by eddiejaoude at 2013-02-21T13:42:30Z Another way to look at it, is if the property has a 'setter' why should it not have a 'getter' too? i.e. If the developer can 'set' it, why cant they 'get' it too. Just another thought, once the best way to move forward is confirmed, I will update my other tests accordingly & submit them. --------------------------------------------------------------------------- by Baachi at 2013-02-21T13:49:25Z Another solution would be, to extend the `Dumper` class and move the `getIndentation` to this class. This class should be located into the `tests/` folder. @stof What do you think? --------------------------------------------------------------------------- by stof at 2013-02-21T14:21:54Z @Baachi IMO, the unit test should ensure that we can actually change the indentation of the dumped code (which is what we want to do). We don't bother about being able to get the indentation (we don't even have a method for it currently), we want it to be applied. The Dumper is not a configuration object. It is an object doing some work. So testing that a new getter returns the value will not ensure that changing the indentation is working. --------------------------------------------------------------------------- by Baachi at 2013-02-21T15:07:23Z @stof Ah yes, I understand you. So my solution is wrong, @eddiejaoude should call `setIndentation` and check the dumped yaml` if the string has the right indentation. --------------------------------------------------------------------------- by eddiejaoude at 2013-02-22T07:35:08Z Ok, thanks for the clarification guys. I will get on the case shortly!