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

Restructure builtin tests #2119

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

Restructure builtin tests #2119

wants to merge 1 commit into from

Conversation

plajjan
Copy link
Contributor

@plajjan plajjan commented Jan 24, 2025

  • rename all test files to test_ - I find this very useful when I want to open the corresponding test file using my editors fuzzy file search
  • place all tests in quite small top level function, which makes it easy to look at generate C code
  • avoid exceptions for control flow, instead using simple if-checks and return - simpler code and we don't rely on working exceptions for most code... not that we'd expect exceptions not to work, but it's just unnecessary
  • overall consolidation of tests and sort into where it makes sense
  • more tests for bytes in particular

This is by no means perfect, but it's a start.

@plajjan plajjan enabled auto-merge January 24, 2025 15:36
- rename all test files to test_ - I find this very useful when I want
  to open the corresponding test file using my editors fuzzy file search
- place all tests in quite small top level function, which makes it easy
  to look at generate C code
- avoid exceptions for control flow, instead using simple if-checks and
  return - simpler code and we don't rely on working exceptions for most
  code... not that we'd expect exceptions not to work, but it's just
  unnecessary
- overall consolidation of tests and sort into where it makes sense
- more tests for bytes in particular

This is by no means perfect, but it's a start.
@plajjan plajjan force-pushed the restruct-builtin-tests branch from df5e25a to a52cfa6 Compare January 24, 2025 15:40
@plajjan plajjan disabled auto-merge January 24, 2025 15:50
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.

1 participant