Fix enum regressions #98
Conversation
Previously, for some legacy reasons, the code for compiling parameters stored
the array of possible values in a structure like this: [{'value': 123}, ...]
@kylef did not know that and did not notice this when rewriting the code
to use the minim library internally, so his changes worked with plain arrays
of values instead: [123, ...] This broke the subsequent code. Interestingly,
neither Dredd Transaction tests or Dredd tests caught this mistake. Especially
Dredd Transactions integration tests were not checking for extraneous warnings
and errors, so the problem slipped through unnoticed. This commit changes the
internal structure for storing the array of values to a plain array.
#94
URI parameters in form of arrays are not officially supported in the API Blueprint parser yet. Even when using Swagger, I found problems with the current implementation, which prevent Dredd from correctly expanding the URI Templates. The feature is completely untested on Dredd side. For that reason, I re-opened the relevant issue on GitHub and hereby I'm demoting array URI parameters to an experimental feature, which is not officially fully supported yet. apiaryio/dredd#791 apiaryio/drafter#500 grncdr/uri-template#17
honzajavorek
force-pushed
the
honzajavorek/enum-regressions
branch
from
25aa182
to
e47bc4d
Compare
| ) | ||
| ) | ||
|
|
||
| it('is compiled into zero transactions', -> | ||
| assert.equal(transactions.length, 0) | ||
| assert.deepEqual(compilationResult.transactions, []) |
There was a problem hiding this comment.
Just curious - why do you use assert.deepEqual(...) instead of assert.equal(compilationResult.transactions.length, 0)?
There was a problem hiding this comment.
Because when this fails, I can see the extra transaction in the test report. Otherwise I'd just see 3 ≠ 0, which isn't very helpful and requires me to go to the test and add console.logs to see what exactly is wrong.
test/integration/compile-test.coffee
Outdated
| transactions = undefined | ||
| # | ||
| # The API description documents used as fixtures to test this situation | ||
| # can also cause errors. We do not care about those in this test. |
There was a problem hiding this comment.
It would be good to add why we do not care about it.
test/integration/compile-test.coffee
Outdated
| w.component isnt 'uriTemplateExpansion' and | ||
| w.component isnt 'apiDescriptionParser' | ||
| ).length, 0) | ||
| ) | ||
| context('the last warning', -> | ||
| it('comes from URI expansion', -> | ||
| assert.equal(warning.component, 'uriTemplateExpansion') | ||
| assert.equal(compilationResult.warnings[-1..][0].component, 'uriTemplateExpansion') |
There was a problem hiding this comment.
I have no idea what array[-1..] magic does. Is there a way to make this more explicit?
There was a problem hiding this comment.
The block says the last warning, but yes, I can save it to an extra variable for each of the its. I agree it's not very intuitive.
| assert.equal(compilationResult.transactions.length, 0) | ||
| ) | ||
| it('is compiled with no warnings', -> | ||
| assert.deepEqual(compilationResult.warnings, []) |
There was a problem hiding this comment.
I think it would be better to keep things consistent and use assert.equal(array.length, expectedLength) when checking for expected number of <something>.
There was a problem hiding this comment.
Same as above, this allows me to see what's there (and what shouldn't be there), directly in the test report. Also, if array is undefined for some reason, array.length blows up.
Addressing @michalholasek's comments under #98
honzajavorek
force-pushed
the
honzajavorek/enum-regressions
branch
from
3921cbd
to
1cabcb8
Compare
|
@michalholasek Added 1cabcb8 to address your comments. The commit should also remove the last occurrences of loose assertions (such as "there are some warnings..." and testing just one). |
|
I found one more type of a loose assertion. There will be one more commit (small). |
|
Added 5defd54. I promise, that was the last commit 😄 |
honzajavorek
force-pushed
the
honzajavorek/enum-regressions
branch
from
01ac1ba
to
5defd54
Compare
|
@honzajavorek Ship it 🚀 |
This is a PR fixes various regressions (such as apiaryio/dredd#872) caused by #94 and maybe also other recent changes and dependency upgrades.
I recommend going commit-by-commit during the review 🔍 Changes broken down to commits:
.skip()-ped in the commit.Merging this should result in automatic release of a patch version of Dredd Transactions, which should fix the apiaryio/dredd#872 the same way as it was introduced.