Unit tests for AttributeUpdates #79
Conversation
|
@amihaiemil Thanks, let me find someone who can review this pull request |
|
@mkordas please review this PR |
|
@amihaiemil I'll check it soon |
Right, makes sense, good catch. |
| /** | ||
| * AttributesUpdates.put(...) throws exception when called. | ||
| */ | ||
| @Test(expected = UnsupportedOperationException.class) |
There was a problem hiding this comment.
@amihaiemil please see: yegor256/qulice#668 - no big harm here, but future version of Qulice would prohibit such code
|
@amihaiemil see my review above |
|
@amihaiemil will you have time for this one today or tomorrow? Should I keep pinging you? |
|
@mkordas tomorrow evening or on sunday morning :D |
|
@amihaiemil any success today? :) |
|
@mkordas done. Can you merge pls? |
| new AttributeUpdates().clear(); | ||
| Assert.fail("#clear should not be supported."); | ||
| } catch (final UnsupportedOperationException ex) { | ||
| MatcherAssert.assertThat("test passed", Matchers.notNullValue()); |
There was a problem hiding this comment.
@mkordas These always-true asserts are done since catch block cannot be empty and also a boolean flag and an if() statement afterwards cannot be done because every variable has to be final.
There was a problem hiding this comment.
@amihaiemil I'm not aware of rule demanding that all variables must be final
|
@amihaiemil I'll check it in an hour |
|
@mkordas sure, thanks |
|
@amihaiemil I'm ready here :) |
| new AttributeUpdates().putAll(new AttributeUpdates()); | ||
| Assert.fail("#putAll should not be supported."); | ||
| } catch (final UnsupportedOperationException ex) { | ||
| MatcherAssert.assertThat("test ok", Matchers.notNullValue()); |
There was a problem hiding this comment.
@amihaiemil that assertion is a bit awkward for me. Personally I'd go with // expected comment or if comment violates Qulice as well I'd make asssertion on ex.getMessage - whatever it is
|
@amihaiemil I have one minor comment open, please take a look when you can, but in the meantime I can trigger merge |
|
@rultor merge |
|
@mkordas thank you. I added the boolean and if() statement. I could have sworn all variables had to be final... strange |
|
@yegor256 ping |
|
@amihaiemil thanks for the fix in 75e8167, looks good |
|
@yegor256 ping |
|
@yegor256 please review this one |
|
@yegor256 this is waiting as well |
|
@yegor256 ping |
|
@yegor256 ping again |
|
@rultor try to merge |
|
@rultor please deploy |
PR for #55 .
Wrote unit tests for the methods in class AttributeUpdates .
Unit test
AttributeUpdatesTest.containsValue()looks weird. Normally it should work with onlyattr.containsValue(value), but it does not. I reported this issue for it.I could have written the correct text (without the "hack") and
Ignoreit, but I didn't want to create dependencies between tasks... If the bug I reported will be accepted, then the test will be "fixed" with it - and if for any reason it won't be accepted, then this task is free, there won't be a need to create other issue just to unIgnore the test.