Skip to content

Fix CI failing tests#464

Open
ivansg44 wants to merge 29 commits intomasterfrom
fix_ci
Open

Fix CI failing tests#464
ivansg44 wants to merge 29 commits intomasterfrom
fix_ci

Conversation

@ivansg44
Copy link
Copy Markdown
Member

@ivansg44 ivansg44 commented Feb 20, 2025

Edit: I should pull the latest changes from master before removing wip label.

@ivansg44 ivansg44 added the wip Work in progress label Feb 20, 2025
Comment thread lib/Validator.js
Comment on lines -327 to -329

// Here slotType value tested and is ok!
continue;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With continue here, the anyOf, allOf, etc. processing code further down inside this for block was not being reached.

Comment thread lib/Validator.js Outdated
Comment on lines 343 to 348
// 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');
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep two error messages, but let's make them more descriptive. Which anyOf conditions are they violating?

Comment thread package.json
Comment thread babel.config.js
Comment thread lib/utils/fields.js
Comment on lines +190 to +195
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;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread tests/fields.test.js
Comment thread tests/templates.test.js
@ivansg44 ivansg44 requested a review from ddooley February 20, 2025 23:39
@ivansg44 ivansg44 added wip Work in progress and removed wip Work in progress labels Feb 25, 2025
Comment thread lib/Validator.js
Comment on lines +296 to +297
// We do not process any nested anyOf, allOf, etc.
return parse_error
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Comment thread lib/Validator.js
Comment thread lib/utils/fields.js
Comment thread yarn.lock
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed rollup-jest

Comment thread .eslintrc.js
Comment thread lib/Toolbar.js
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of this stuff was done automatically by Prettier. I renamed a variable from className to _className

@ivansg44 ivansg44 removed the wip Work in progress label Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants