Fix CI failing tests #464
Fix CI failing tests #464
Conversation
lib/Validator.js
Outdated
|
|
||
| // Here slotType value tested and is ok! | ||
| continue; |
There was a problem hiding this comment.
With continue here, the anyOf, allOf, etc. processing code further down inside this for block was not being reached.
lib/Validator.js
Outdated
| // If every condition gives format error, just return one error | ||
| // TODO does this apply in allOf, second branch of exactlyOneOf? | ||
| if (results.every((result) => result === parse_error)) { | ||
| return parse_error; | ||
| } | ||
| return results.join('\n'); |
There was a problem hiding this comment.
Test:
expect(fn('hello')).toEqual('Value does not match format for xsd:integer')
We were getting 'Value does not match format for xsd:integer\Value does not match format for xsd:integer' without this new if condition.
Was the test wrong? Do want duplicated format error messages across two anyOf conditions? If so, I would remove this if block and edit the test.
Or do we want to keep this if block, and add a similar one for allOf, and the second return branch of exactlyOneOf?
There was a problem hiding this comment.
Keep two error messages, but let's make them more descriptive. Which anyOf conditions are they violating?
| "transform": { | ||
| "\\.js$": [ | ||
| "rollup-jest" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Do we need this? It leads to source mapping issues, with test error messages pointing to the wrong line. It also becomes impossible to step through the tests using a debugger (at least in JetBrains). The pointer stops at random places.
| module.exports = { | ||
| presets: [['@babel/preset-env', {targets: {node: 'current'}}]], | ||
| }; |
There was a problem hiding this comment.
This file is only necessary if we remove rollup-jest. We should also uninstall rollup-jest if we do that.
| const slotName = fieldSymbolsAtIndex[index].slotName; | ||
| let slotTitle = fieldSymbolsAtIndex[index].slotTitle; | ||
| // If slotTitle not specified; use slotName as slotTitle | ||
| slotTitle = slotTitle === undefined ? slotName : slotTitle; | ||
|
|
||
| tempObject[slotTitle] = slotName; |
There was a problem hiding this comment.
Is this what we want? A test was failing previously because the fields global var in fields.test.js was missing slotTitle values. There are several lines of code downstream of this function that expect slotTitle values. I just re-used slotName as title as well to fix things.
| expect(dataArray).toEqual(['', '33.333', '', '', 'a; b; c', '33', '', '']); | ||
| expect(dataArray).toEqual(['', '33.333', '', '', 'a;b;c', '33', '', '']); |
There was a problem hiding this comment.
The delimiter downstream is just a semicolon, not a semicolon and a space. If we do indeed want it to be semicolon and a space, I need to discard this change, and update the delimiter downstream instead.
| // // Mocking the manifest data | ||
| // jest.mock('@/web/templates/manifest.json'); |
There was a problem hiding this comment.
This mock module import was leading to a configuration error. I noticed every other manifest reference in this file was commented out, so I comment the mock import out. Do we need this? The module import error may also have been due to me messing with jest-rollup. Not sure.
There was a problem hiding this comment.
I'm guessing we don't need it. I removed all references to manifest.json at begining of month...
| // We do not process any nested anyOf, allOf, etc. | ||
| return parse_error |
There was a problem hiding this comment.
a_big_or_small_number: {
name: 'a_big_or_small_number',
range: 'integer',
any_of: [{ maximum_value: 10 }, { minimum_value: 100 }],
}
'hello' will return a parse error === 'Value does not match format for xsd:integer'
The test suite expects us to return this error immediately, so we must return parse error here. Otherwise we will try to process the any_of unnecessarily.
Previously, there was a continue further down that I think was meant to stop such unnecessary processing of any_of, all_of, etc. But I had to comment it out because it was also stopping processing in situations without an initial parse error--where we actually want to process any_of, all_of, etc. e.g., '11' should return 'Value is greater than maximum value\nValue is less than minimum value'
| // The below code was preventing the remaining code from being | ||
| // reached, which included processing any_of, all_of, etc. | ||
| // // Here slotType value tested and is ok! | ||
| // continue; |
There was a problem hiding this comment.
See above comment.
| // Consider the following situation: | ||
| // `key === undefined` | ||
| // `f = {'name': 'a', 'title': 'a'}` | ||
| // If we do not check whether `f` has `name` or `alias`, then: | ||
| // 1) `f.name === key || f.alias === key` | ||
| // 2) `'a' === undefined || undefined === undefined` | ||
| // 3) `false || true` | ||
| // 4) `true` --> direct match | ||
| // i.e., we directly match `undefined` to a defined field with name `a` |
There was a problem hiding this comment.
This was not directly causing a failing test, but it is an issue I identified while debugging some other code that was causing a failing test. There should have been a console warning for that other failing test "Failed to direct match ...", but it was not being emitted for the failing test due to the above bug.
There was a problem hiding this comment.
We were erroneously saying an undefined key was a direct match to some defined fields that were simply missing an alias attribute.
yarn.lock
Outdated
There was a problem hiding this comment.
Removed rollup-jest
| rules: { | ||
| 'no-unused-vars': ['error', { | ||
| 'argsIgnorePattern': '^_', | ||
| 'varsIgnorePattern': '^_' | ||
| }], | ||
| }, |
There was a problem hiding this comment.
Stops prettier from throwing an error for unused vars that start with an underscore. Was necessary in Toolbar.js, because there was an unused var className. I renamed it to _className
There was a problem hiding this comment.
A bunch of this stuff was done automatically by Prettier. I renamed a variable from className to _className
Edit: I should pull the latest changes from master before removing wip label.