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

Feature/execute failing command #247

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Yanqiao4396
Copy link
Collaborator

@Yanqiao4396 Yanqiao4396 commented Oct 17, 2022

A new check to ensure the wrong command should return exit code 1 as expected. As long as it crashes and has return code 1 as expected, the related check should pass

What is the current behavior?

This feature is achieved by a new check type: ExecuteFailingCommand with the necessary flag: command.
Unite test is passed and the code is proved to be vital in gatorgrade, which is shown in the screenshot below
Screen Shot 2022-10-17 at 10 36 47 AM

What is the new behavior if this PR is merged?

Other information

This PR has:
  • Commit messages that are correctly formatted
  • Tests for newly introduced code
  • Docstrings for newly introduced code

This PR is a feature

Developers

@Yanqiao4396 @jnormile @burgess01

@gkapfham
Copy link
Collaborator

Hello @Yanqiao4396, thanks for creating this PR and sharing details about the feature. Quick questions:

  • Have you done integration testing with this feature by using gatorgrade to invoke it?
  • If you have, then can you show what the configuration file would look like in gatorgrade.yml?
  • If you have, then can you show what would be the output from running this command?
  • If you have not, then do you think that it would be possible to perform some type of integration testing?

Please note that it would be fine if you perform some type of manual integration testing! To be clear, I'm not requesting that you implement and run any type of automated integration tests. Although, that type of test suite is sorely needed!

@Yanqiao4396
Copy link
Collaborator Author

Yanqiao4396 commented Oct 18, 2022

@gkapfham , you could check the screenshot I attached above if it makes sense to you, it's gatorgrade other than gatorgrader which I was running there. It mimiced the process of invoking gatorgrader with arguments list. So I may think it's integration test to show gatorgrade can call gatorgrader as a dependency with the new check ExecuteFailingCommand. So somehow, I feel I've done automated integration tests there even if I didn't push the test into gatorgrade. I didn't because there is no specific test case for specific check in gatorgrade level.
When I said unit test, I may I created the test functions for the source codes I changed in gatorgrader. Sorry for confusion.

@Yanqiao4396 Yanqiao4396 reopened this Oct 18, 2022
@burgess01
Copy link
Collaborator

burgess01 commented Nov 18, 2022

@gkapfham, I locally installed this branch's version of Gatorgrader and locally installed a version of gatorgrade that didn't have the specified version of gatorgrader listed. I used the command shell lab from operating systems in order to check this, as it was easy to simulate passing and failing builds. Due to this I found a slight error with this code, where since I was using sys.exit() and try/catch blocks to handle errors but still throw a 1 exit code at the end weren't passing with a failure. Through further testing, this turns out to be because it wasn't passing the requirement for both a failing build AND an error message, so I changed that to be an or statement, as realistically if either of those happens that means the build failed. Attached are the results of running the new tests I created as well as the code for the tests. If you need anything else to show that our code is now working I would be happy to provide it!

Screen Shot 2022-11-18 at 3 55 44 PM

@gkapfham gkapfham added the on-hold This PR or issue may be handled in the future label Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature on-hold This PR or issue may be handled in the future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants