search

exercism / c-test-runner

GNU Affero General Public License v3.0
5 stars 5 forks source link

When should we run tests with address sanitizer? #11

Open elyashiv opened 4 years ago

elyashiv commented 4 years ago

Should we run every exercise with address sanitizer or would it confuse the new students?

Can we filter the results or add a comment to explain whats happening? Can we run the address sanitizer just after a certain exercise/just on exercises that really need allocation?

siebenschlaefer commented 3 years ago

IMHO we should definitely run the address sanitizer. I've see too many solutions with memory leaks or out-of-bounds accesses. I think the question is: How do we present that to the students?

wolf99 commented 3 years ago

iHiD has indicated that this should be part of the analyzer. https://github.com/exercism/c-test-runner/issues/10#issuecomment-665059910 However I think the C track does not have an anlyzer yet. Also not concept exercises.

I think getting either of those in place is a pre-requirement to adding more testing. Could really do with some contributions to get concept exercises added as I have not had enough time to really focus enough to get started with that.

EDIT:

I see on slack, that @iHiD and @ErikSchierboom have now recommended adding this in the test-runner. Double-checking there, but otherwise, please go ahead and add it here. Would still also dearly like anyone to contribute concept exercises also! 😉

iHiD commented 3 years ago

I'm pretty agnostic to where things live as long as someone working via the CLI can have the same testing experience as someone using the website. So as long as a student can also run it locally (and are told to do so) as part of the test instructions, then I'm happy for it to be in the test runner :)

ErikSchierboom commented 3 years ago

So as long as a student can also run it locally (and are told to do so) as part of the test instructions, then I'm happy for it to be in the test runner :)

☝️ Should this not be possible, I think the sanitizer should be part of the analyzer.

siebenschlaefer commented 3 years ago

Currently each makefile has the targets test and memcheck. test compiles the solution, the tests, and the test framework, links them and executes the tests. Nothing fancy. memcheck does the same as test but with extra flags for the compiler and linker that enable the AddressSanitizer.

In many of the over 800 solutions I've mentored I found memory leaks or out-of-bounds reads, and I frequently told my students to run make memcheck.

Now that I think about it: Why is memcheck an extra target? Can't we just always use the AddressSanitizer?

ErikSchierboom commented 3 years ago

Can't we just always use the AddressSanitizer?

To me, that sounds like a very good plan.

wolf99 commented 3 years ago

We can, but I second the request from #10

If it was explained first for the student, ... it would be much more useful. ... "here's how C lets you do dangerous things with memory, here's why you don't want to do that, and here is a tool (ASAN) that we'll now run on the exercises to make sure those problems are caught".

I think tat for a novice, seeing a stack trace for the first time it is not so easy to figure what the tool is trying to tell you, never mind what it is they need to do about it. Track documentation will be key.

ryanplusplus commented 3 years ago

I think tat for a novice, seeing a stack trace for the first time it is not so easy to figure what the tool is trying to tell you, never mind what it is they need to do about it. Track documentation will be key.

I can confirm that most junior professional C developers struggle to digest ASAN's output. As a learner, it's probably preferable to have a human deliver the bad news the first few times.

ryanplusplus commented 3 years ago

Can't we just always use the AddressSanitizer?

To me, that sounds like a very good plan.

I'm pretty sure that ASAN doesn't exist yet for MacOS on ARM. Do we know if that's a large part of our userbase at this point?

ErikSchierboom commented 3 years ago

I think tat for a novice, seeing a stack trace for the first time it is not so easy to figure what the tool is trying to tell you, never mind what it is they need to do about it. Track documentation will be key

I made my comment without knowing what the output looks like. If you think the output is hard for people to parse, could you do the memcheck in the C analyzer and make its output be easier for students to understand? That way you could also include additional links and such.