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

Add Jest for Basic Testing of Pure Functions #1033

Merged
merged 6 commits into from
Sep 21, 2023

Conversation

JGreenlee
Copy link
Collaborator

One example test given here - simply asserting an expected output from a function of diaryHelper.

This will only allow us to test simple functions, not components

JGreenlee and others added 2 commits September 18, 2023 15:46
testing the testing framework :)

my goal is to test the normal cases and the edge cases

ex: isMultiDay - test one expecting true, one expecting false, and one with invalid input (so false)
@Abby-Wheelis
Copy link
Member

I just got a chance to test this out, and am working on adding a few more tests. This is my first time working with this testing framework, so it took me a moment to figure out. I discovered that the tests can be run by setting up the phone server (I needed to do a full re-setup to have the updated dependencies) and then typing "npm test" at the command line.

@JGreenlee can you take a look at the test I commented out and see if you think it should run? It's failing for me, but I haven't figured out why.

@JGreenlee
Copy link
Collaborator Author

No fault of the test. That function isn't working as intended!
Look at the body of the function and you see that key is never used. So the function won't work with something like MotionTypes.WALKING.

All the more reason that we need tests to find unexpected behaviors like this!

/**
 * @param motionName A string like "WALKING" or "MotionTypes.WALKING"
 * @returns A MotionType object containing the name, icon, and color of the motion type
 */
export function motionTypeOf(motionName: MotionTypeKey | `MotionTypes.${MotionTypeKey}`) {
  let key = ('' + motionName).toUpperCase();
  key = key.split(".").pop(); // if "MotionTypes.WALKING", then just take "WALKING"
  return MotionTypes[motionName] || MotionTypes.UNKNOWN;
}

As discovered in e-mission#1033 (comment), this function was not working as expected - it formats a key for lookup, and then didn't actually use the key.
With this fix, the test runs correctly!
@JGreenlee
Copy link
Collaborator Author

Runs correctly with fix, added commit

@JGreenlee
Copy link
Collaborator Author

@shankari How can we integrate this with Github Actions so the tests run automatically?

@shankari
Copy link
Contributor

shankari commented Sep 20, 2023

@JGreenlee you would create a new github actions workflow (.github/workflows) that runs the command. I would suggest starting with the install workflow osx-serve-install from this repo and adding on a test stage, similar to the way that the test-with-manual-install builds on osx-ubuntu-manual-install in the server code.

at this point, I think that @nataliejschultz is our GitHub actions expert since she has actually written a new workflow, although both @humbleOldSage and @MukuFlash03 have been adding tests as well.

Abby Wheelis and others added 2 commits September 20, 2023 10:43
continuing to improve testing coverage for diaryHelper by adding tests for more functions

For some of the functions related to a trip, I found it easiest to look at a trip in the logs, create a test trip object with the relevant parts, and then also mock the results
@JGreenlee
Copy link
Collaborator Author

That seems to have worked! I think the tests ran twice though (once for the www folder, and once for the platforms/browser/www folder)
I'll see if I can ignore platforms so actions don't take longer than they need to

We should only look for tests in our `www/__tests__` directory, not any other `__tests__` directories that may exist in dependencies or anywhere in the project directory tree.
Further, by explicitly ignoring these directories, we don't have to specify `npx jest www/__tests__`, and can simply run `npx jest`.
@JGreenlee JGreenlee marked this pull request as ready for review September 20, 2023 22:08
Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

I am merging this for now, since the workflow name does not affect the addition of new tests, and we want people to write tests as they are migrating the code.

@JGreenlee please create a new workflow or rename this one in a future PR

Copy link
Contributor

Choose a reason for hiding this comment

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

In the server code, we have install and test in separate workflows.
(future fix): I am fine with having this in the same workflow (at least initially), but then we should rename the file to indicate that it is also testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Address code reviews with "future"
Development

Successfully merging this pull request may close these issues.

3 participants