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

Linked list tests #119

Closed
wants to merge 14 commits into from
Closed

Linked list tests #119

wants to merge 14 commits into from

Conversation

Jeroenvh99
Copy link
Contributor

I wrote some basic tests for the linked list functions, tbh it was more to practice a bit with testing

Copy link
Collaborator

@W2Wizard W2Wizard left a comment

Choose a reason for hiding this comment

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

I'd honestly much rather have a resizable array over linked lists one day so I am questioning the need of the linked lists themselves.

Also the need to the test the lists is not really that important more so in relation to images. Say, we add a image we delete a image and expect the list to look a certain way.

tests/tests.cpp Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall, I sort of get it but also this is not how the testing framework is supposed to be used.

Please take a look at the test.cpp file so see how asserts are being used.

@Jeroenvh99
Copy link
Contributor Author

Well, this was more of an exercise for myself to practice different testing techniques, so it doesn't really add a lot of value to MLX I guess

@W2Wizard
Copy link
Collaborator

W2Wizard commented Nov 6, 2023

For now I won't merge this as mentioned for the reasons above.

@W2Wizard W2Wizard closed this Nov 6, 2023
@Jeroenvh99
Copy link
Contributor Author

That's fair, it doesn't really add much value to MLX as it is

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.

2 participants