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

Update test_malloc_free.sh #11

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

Conversation

robertnen
Copy link

- now it checks if the executable give segmentation fault or not and gives 0 points for it
@teodutu teodutu requested a review from razvand October 24, 2023 06:05
@teodutu teodutu added the student-contrib Fix or improvement made by a student label Oct 24, 2023
@razvand razvand self-assigned this Oct 24, 2023
@razvand
Copy link

razvand commented Oct 24, 2023

Thanks, @Lucky8boy , for submitting it. It's very good to improve the tests in order to make them more resilient and increase the coverage of cases.

However, checking the executable for Segmentation fault is not the proper way to do it. The proper way is to test for the cause of the PR. In other words, the test should be giving 0 points NOT because a Segmentation fault appears, but because there is something wrong in the program logic.

The proper update to the test would be to give 0 points by noting that the program doesn't do the right thing. If that's a segmentation fault, it's not that relevant.

What you should be looking at what is the cause of the segfault, and why is the current test not considering that cause. Maybe it's not checking for a return value, or anything like that. The process disappearing is a good to check as well.

In all cases, even if segfault is the only way to check this (I'm highly doubtful this is the case), using Valgrind is too heavyweight.

@github-actions github-actions bot added the area/assignment Update to homework assignments label Oct 24, 2023
@teodutu teodutu added the kind/improve Improve / Update existing content / item label Oct 30, 2023
@Alex-deVis
Copy link

@razvand, bump on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/assignment Update to homework assignments kind/improve Improve / Update existing content / item student-contrib Fix or improvement made by a student
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants