Skip to content
This repository has been archived by the owner on Sep 25, 2019. It is now read-only.

feat(scripts): add testing for React/Redux challenges #308

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ojeytonwilliams
Copy link
Contributor

ISSUES CLOSED: #305

Description

This includes modern challenges in the test suite. The test suite in curriculum is now closer to the test runner learn though it still ignores async tests.

Pre-Submission Checklist

  • Your pull request targets the dev branch.
  • Branch starts with either fix/, feature/, or translate/ (e.g. fix/challenge-tests)
  • All new and existing tests pass the command npm test.
  • Use npm run commit to generate a conventional commit message.
    Learn more here: https://conventionalcommits.org/#why-use-conventional-commits
  • The changes were done locally on your machine and NOT GitHub web interface.
    If they were done on the web interface you have ensured that you are creating conventional commit messages.

Checklist:

  • Tested changes locally.
  • Addressed currently open issue (replace XXXXX with an issue no in next line)

@@ -38,6 +38,9 @@ const schema = Joi.object().keys({
isRequired: Joi.bool(),
name: Joi.string(),
order: Joi.number(),
react: Joi.bool(),
reactRedux: Joi.bool(),
Copy link
Member

Choose a reason for hiding this comment

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

Why are you adding these properties to the schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

evaluateTest uses them to determine if it needs to add react, redux and so on to the test environment. In order for that to happen, they have to be present in the challenge object (or the validation step will fail) and that means they have to be in the schema.

Copy link
Member

Choose a reason for hiding this comment

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

Understood, is there a way to strip them out of the challenges on build? We are trying to keep the package size to a minimum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, you don't want these properties making their way into the dist folder - since they're not used outside of testing. That can be done, by modifying the gulpfile, but the build process doesn't (currently) involve validation. In other words, the schema isn't enforced.

Does it seem reasonable to add filtering and validation to the build process? That way all the properties that're currently omitted when learn reads the challenges instead never make it into the build of curriculum.

@@ -36,7 +36,10 @@
"config": "commitizen.config.js"
}
},
"dependencies": {},
"dependencies": {
"enzyme-adapter-react-16": "^1.5.0",
Copy link
Member

Choose a reason for hiding this comment

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

These should be devDependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, stupid mistake on my part. Should I git commit --amend it or is it best to just push another commit?

Copy link
Member

Choose a reason for hiding this comment

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

Another commit is fine

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modern challenge types are not tested
2 participants