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 self.assertNumQueries to test #1971

Closed

Conversation

crccheck
Copy link

Description

Please include a summary of the change and which issue is fixed. Please also
include relevant motivation and context. Your commit message should include
this information as well.

Fixes # (issue)

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

@tim-schilling
Copy link
Member

@crccheck I think this is a WIP. I'm inclined to close it to keep the open PRs minimal until there's more of a direction for the work that requires discussion/review.

@matthiask
Copy link
Member

I think there's some value in asserting how many quietus Django sees vs how many queries the toolbar sees. I think both functionalities use the same underlying data structure, which makes this additional assertion not very useful. Or maybe I'm missing something?

@tim-schilling
Copy link
Member

@crccheck I'm going to close this. If you feel like this is important, please let us know why and we can reopen it.

@crccheck
Copy link
Author

oh sorry, I only opened it to see if CI would run it. I figured out how a minimal command to get tests to run locally since then and I thought draft PRs wouldn't show up

@crccheck
Copy link
Author

FYI @matthiask there's an issue where Django's queries actually don't match Debug Toolbar #1791 I was trying to recreate

I think there's some value in asserting how many quietus Django sees vs how many queries the toolbar sees. I think both functionalities use the same underlying data structure, which makes this additional assertion not very useful. Or maybe I'm missing something?

@tim-schilling
Copy link
Member

Ah, that makes sense. Thanks for following up!

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