curriculum icon indicating copy to clipboard operation
curriculum copied to clipboard

Introduction to React Testing: Add missing vitest import statements to testing example.

Open dylen400mh opened this issue 1 year ago • 6 comments

Because

The testing example from the Introduction to React Testing lesson is missing the vitest import statement, leading to describe, it, and expect not being defined when copy-pasting the example code to follow along.

This PR

  • Adds the following import statement to App.test.jsx and App.test.js examples:
import { describe, it, expect } from "vitest";

Issue

N/A

Additional Information

N/A

Pull Request Requirements

  • [x] I have thoroughly read and understand The Odin Project Contributing Guide
  • [x] The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • [x] The Because section summarizes the reason for this PR
  • [x] The This PR section has a bullet point list describing the changes in this PR
  • [ ] If this PR addresses an open issue, it is linked in the Issue section
  • [x] If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • [x] If any lesson files are included in this PR, they follow the Layout Style Guide

dylen400mh avatar Sep 06 '23 23:09 dylen400mh

@TheOdinProject/react I'm not sure why I can't assign a React team member to this PR, but could one of you take a look at this PR.

rlmoser99 avatar Sep 07 '23 11:09 rlmoser99

Hm, in Jest this just works without needing to import describe/it/expect. Is that not the case with vitest @01zulfi?

wise-king-sullyman avatar Sep 13 '23 00:09 wise-king-sullyman

@dylen400mh If you set globals to true in vite.config.ts file, these imports will be global, so you don't need to import these in each file.

// vite.config.ts file
export default defineConfig({
  plugins: [react()],
  test: {
    globals: true, 
    environment: 'jsdom',
    setupFiles: './tests/setup.ts',
  },
})

zhna123 avatar Sep 14 '23 13:09 zhna123

If you follow the setup instructions, the globals will indeed be set to true. The warning you're seeing comes from ESLint, but when you run npm run test, the tests should work.

Screenshot 2023-09-16 at 23 57 26

Maybe we should consider some instructions on updating ESLint rules to avoid the linter warnings?

ManonLef avatar Sep 16 '23 21:09 ManonLef

Yeah, looks like we'll need to calm eslint down. I'll take a look soon

01zulfi avatar Sep 19 '23 15:09 01zulfi

@01zulfi was this something we still need to address?

thatblindgeye avatar Feb 11 '24 14:02 thatblindgeye

Just tested this myself and found no eslint issues.

Ran npm create vite@latest foo -- --template react to scaffold an app, then inside that app did all of the lesson instructions and nothing more. Can confirm doing this raises no eslint errors. That includes checking the ESLint VSCode extension both with the release and pre-release version, and without the Use Flat Config setting checked (since Vite still scaffolds with ESLint v8.57.0 and .eslintrc, due to eslint-plugin-react not yet supporting v9).

Would someone else be able to check this running through the same instructions? If they can confirm the same "no issues" behaviour, then we needn't action this and can close this PR, if it may have just been an "old version of ESLint thing"

MaoShizhong avatar May 08 '24 20:05 MaoShizhong

Closing at it doesn't seem this is an issue anymore, and I can't say I've seen it mentioned at all recently in the community. A new issue can always be opened if this changes.

MaoShizhong avatar Jun 02 '24 00:06 MaoShizhong

Reopening this because my previous testing was not thorough, as discovered when helping a learner with this issue. I did test the release version of the ESLint extension without setting flat config support in VSCode settings, and this seemed fine but it turns out I didn't actually restart the extension when switching from the pre-release version. Restarting the extension highlights this still being an issue, because the eslint config does not recognise Vitest globals.

Adding jest: true to the env object in the eslint config fixes most of the globals, but won't cover stuff like vi, since they're not part of Jest. That would require a manual exception in a globals object (ESLint doesn't have a vitest env option).

I propose the most appropriate way to resolve this would be to add the explicit imports in the lesson examples, and also add a note box saying we're doing this despite the previous setup tutorial using globals: true because of the ESLint extension. Importing would also provide the typedefs for better intellisense anyway. Making eslint aware of the globals will just set them as any.

@dylen400mh I know it's been a while but is this still something you'd like to work on? If so, the following needs to be done:

  • Sync your branch with upstream main and resolve any merge conflicts that arise
  • Add a note box immediately after the first test example saying something like
    <div class="lesson-note" markdown="1">
    
    #### Vitest globals and ESLint
    
    Even if you set `globals: true` in `vite.config.js` like in the setup tutorial, ESLint may still yell at you, as it will not recognize these globals without some extra stuff in your ESLint configuration file. The most straightforward resolution would be to explicitly import the globals you'd need.
    
    </div>
    

MaoShizhong avatar Jun 05 '24 13:06 MaoShizhong

Closing due to no response from OP. Will open a new issue for this, seeking assignment.

MaoShizhong avatar Jun 21 '24 12:06 MaoShizhong