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.

Sign up for GitHub

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

Jump to bottom

Tests pass (but with console errors and warnings) #357

Merged
merged 2 commits into from
Jan 31, 2024
Merged

Tests pass (but with console errors and warnings) #357

merged 2 commits into from
Jan 31, 2024

Conversation

gabalafou
Copy link
Contributor

@gabalafou gabalafou commented Jan 24, 2024 •
edited
Loading

I couldn't find any GitHub issue about the Jest tests being broken.

Description

This pull request:

  • Allows yarn test to run and exit without errors
  • (But does not fix any of the uncaught errors that get generated during test execution and logged to the console but do not actually cause any of the tests to fail.)

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

gabalafou
gabalafou commented Jan 25, 2024
View reviewed changes
src/theme.tsx Outdated
@@ -149,6 +149,8 @@ export const condaStoreTheme = createTheme(baseTheme, {
}
});

export const theme = condaStoreTheme;
Copy link
Contributor Author

@gabalafou gabalafou Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If preferable I could change the import { theme } lines in the tests to import { condaStoreTheme }, instead of exporting theme in addition to condaStoreTheme

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any fundamental change/impact of one or the other approach aside from a double export?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, me asking the question was really a tell that I shouldn't alias the theme. I pushed a commit that fixes the tests without doing this. It wasn't hard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the reason why I think I shouldn't alias it is because it could make it harder to trace where that particular theme is being used across the code base (much easier to search the string "condaStoreTheme" than "theme")

Copy link
Collaborator

Choose a reason for hiding this comment

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

it makes sense, thank you

@pavithraes pavithraes added type: bug 🐛 Something isn't working area: testing ✅ Design of the test infrastructure and everything related needs: review 👀 labels Jan 26, 2024
trallard
trallard approved these changes Jan 30, 2024
View reviewed changes
Copy link
Collaborator

@trallard trallard left a comment

Choose a reason for hiding this comment

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

Thanks @gabalafou left a couple of questions mostly out of curiosity but none of these are blocking.

src/theme.tsx Outdated
@@ -149,6 +149,8 @@ export const condaStoreTheme = createTheme(baseTheme, {
}
});

export const theme = condaStoreTheme;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any fundamental change/impact of one or the other approach aside from a double export?

test/environmentCreate/SpecificationCreate.test.tsx Show resolved Hide resolved
@trallard
Copy link
Collaborator

(But does not fix any of the uncaught errors that get generated during test execution and logged to the console but do not actually cause any of the tests to fail.)

It seems then we'd need a follow-up more focused PR to fix tests then @gabalafou ?

@gabalafou
Copy link
Contributor Author

It seems then we'd need a follow-up more focused PR to fix tests then @gabalafou ?

Maybe. Probably. I'll look into it and get back to you.

@trallard
Copy link
Collaborator

Thanks, @gabalafou. I will go ahead and merge this.

Per #357 (comment), let's circle back on your findings and make a plan/open issues as needed

@trallard trallard merged commit a4aae14 into conda-incubator:main Jan 31, 2024
1 of 2 checks passed
@nkaretnikov nkaretnikov mentioned this pull request Feb 6, 2024
Closed
@gabalafou gabalafou mentioned this pull request Feb 10, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trallard trallard trallard approved these changes

Assignees
No one assigned
Labels
area: testing ✅ Design of the test infrastructure and everything related status: merge ready 🚀 type: bug 🐛 Something isn't working
Projects
conda-store 🐍
Status: Done 💪🏾
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gabalafou @trallard @pavithraes