Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Test Suite: ToDo Module #270

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

Conversation

JoeVanKessel
Copy link

What type of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactor
  • Documentation
  • Not Sure?

Does this PR introduce breaking changes?

  • Yes
  • No

List any relevant issue numbers:

Description:

I more thorough test suite for the todo list module. This test suite focuses on scalability, so any number of tests can easily be created for a given module.

Copy link
Member

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! @JoeVanKessel
Could you explain a little bit more why this change would be useful, compared to the current tests we currently have ?

@@ -0,0 +1,221 @@
function getPossibleExpressions(expressionType, count) {
const data = require('../data/expressions/en.json')
Copy link
Member

Choose a reason for hiding this comment

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

You should not use the CJS require syntax, instead use the ESM import.

return data.todolist.view_lists.expressions
case "view list":
return data.todolist.view_list.expressions
case "rename list":
Copy link
Member

Choose a reason for hiding this comment

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

Instead to return the same things for multiple values of expressionType, you can do something like that :

case "view lists":
case "view list":
  return data.todolist.view_list.expressions

Comment on lines +28 to +29
const fs = require('fs')
const path = require('path')
Copy link
Member

Choose a reason for hiding this comment

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

Instead to use the CJS require syntax, use the ESM import at the top of the file.

function getOutputs(){
const fs = require('fs')
const path = require('path')
let data = JSON.parse(fs.readFileSync(path.resolve(__dirname, '../data/db/calendar.spec.json')))
Copy link
Member

Choose a reason for hiding this comment

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

You should not use let if you don't redeclare the variable, instead use const.
Also instead to use fs.readFileSync, use the promises based API instead, like fs.promises.readFile and you can create a new variable to store path.resolve(__dirname, '../data/db/calendar.spec.json')), it will probably be clearer. 👍


async function testCreateListExpressions(count) {
const expression = getPossibleExpressions("create list")[count]
var quote = '\"'
Copy link
Member

Choose a reason for hiding this comment

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

You should not use var, instead use let or const.

}
}

function createObjs(){
Copy link
Member

Choose a reason for hiding this comment

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

Prefer explicit than implicit, so instead you could call this function createObjects.
Also it doesn't seems like this function "only" create objects, it actually creates todos/lists objects, so be more explicit of what the function does in its name.

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.

3 participants