add second round test; update readme #19
Conversation
There was a problem hiding this comment.
Thanks much for submitting a PR for this project! I'm happy to see that it's still in use. I know that it definitely helped me when I took CS120 :)
I have a few suggestions for improvement here - one of which is the formatting of this PR. It's impossible to see what's actually changed since every line has indentation change.
README.md
Outdated
| Also, for the project since it is running in single CPU, so whoever calls | ||
| EndingProc should also be the one thats running in the CPU, which means the | ||
| p given to the EndingProc should be the pid thats calling it. | ||
|
|
||
| However, this does not apply to the tests, the p given to the | ||
| EndingProc may not be the one thats calling it, so you should consider such | ||
| situation in your code to utilize the test. But also, consider such situation, | ||
| where one process may kill another process, and you can see it is resonable to | ||
| allow such operation in your projcet. | ||
|
|
There was a problem hiding this comment.
To me, it seems like this is helping people "cheat" by giving them a hint on how to do the assignment. I vote that we take this section out of the README. After all, they should figure this out when they run the test ;)
There was a problem hiding this comment.
The reason why I put this into the README is that the test.c doesnt really behave like the actual environment in the schools server, as it only contains one CPU and only ends a process when the process itself calls Exit(). So in other words, the behavior that one process may kill another process is not tested by the autograder, and that`s why I think we can have an explanation here.
So, if you think that it`s giving hint to others it is totally fine to remove this part in README, or we can just replace this part with some simple notes here, like:
In this test program, there is such behavior that the running process may kill another process, which is not tested in the autograder.
I suggest we put the above simple notes here, because since it is not tested in the autograder, we dont need to add difficulty for students who are using the test program.
What do you think?
There was a problem hiding this comment.
I agree. I'm hesitant about the big blurb, but happy about the simple note :)
There was a problem hiding this comment.
Please remove the formatting changes from this PR. Two reasons:
- I'm still having a hard time reviewing what this PR is actually doing since there's tons of formatting changes.
- I'd rather not pollute the history logs and make every line attributed to this commit.
added second round checking for normal tests; modified README.MD