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

Replace for_each_index usage in tests #216

Open
dsharlet opened this issue Apr 4, 2024 · 0 comments
Open

Replace for_each_index usage in tests #216

dsharlet opened this issue Apr 4, 2024 · 0 comments
Labels
enhancement New feature or request good first issue Good for newcomers work ready This task is ready to be worked on

Comments

@dsharlet
Copy link
Owner

dsharlet commented Apr 4, 2024

The use of for_each_index in tests might be causing a few problems:

  • It generates a lot of complicated code for the compiler to generate. pipeline_test.cc takes a long time to compile with --config=asan.
  • Somehow, when an assert fails in a lambda, it seems to only unwind one level of the stack. Inside a callback like for_each_index, this means you can get thousands of assert failure messages, when the test should just fail on the first one.
  • It's hard to debug these tests without being able to look at the buffer data somehow.

We should replace these with something like:

template <typename T>
bool is_buffer_equal_to(const slinky::buffer<T>& a, std::function<T(std::span<const index_t>)>b);

This would allow comparing a buffer to some generated ground truth. We can add some ability to print out slices, and avoid asserting thousands of times.

@dsharlet dsharlet added enhancement New feature or request good first issue Good for newcomers work ready This task is ready to be worked on labels Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers work ready This task is ready to be worked on
Projects
None yet
Development

No branches or pull requests

1 participant