Skip to content

Conversation

@ConnorClancyDeakin
Copy link

Description

The sktest.exe has been unable to work on the MSYS2/MINGW64 it was getting stuck in one test and wasnt giving anyfeedback the terminal would freeze up (if you want to simulate the error you should be able to just run the sktest in you msys2 mingW64) The change that has been provided here changes the way the test is built to be a console exectuable which has made it work on my machine.

Fixes sktest.exe not working in MSYS2/MINGW64

Type of change

Please delete options that are not relevant.

  • [ ✓] Bug fix (change which fixes an issue)

How Has This Been Tested?

first you must be using MYSYS2/WINGW64 although it could be beneficial to check with others as well to make sure it doesnt stop anything else from working

This change can be tested by simply building and testing the program using the sktest.exe file when you do this it should let you choose any test and run them as well as exit the program. The tests should complete and bring you back to the main terminal to choose another test. Essentially you should get a terminal menu when you run the sktest.exe that you can use.

Testing Checklist

  • [✓] Tested with sktest

Checklist

  • [✓] My code follows the style guidelines of this project
  • [✓] I have performed a self-review of my own code
  • [✓] I have commented my code in hard-to-understand areas
  • [✓] I have made corresponding changes to the documentation
  • [✓] My changes generate no new warnings
  • [✓] I have requested a review from ... on the Pull Request

Extra note

This change changes the way the sktest is built as far as I've been able to find there has been no uninteded consequences of this but I havent done a change like this before, it should only effect the sktest.exe when msys is being used but if you know more than me and think this should be different please put that in your peer review as I do not want to break things on accident. essentially please dont be afraid to be harsh.

Copy link

@JPF2209 JPF2209 left a comment

Choose a reason for hiding this comment

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

Summary

  • this code is good enough to approve for the 2nd peer review

Type of Change

  • This piece of code correctly identifies the changes made being a bug fix

Code Readability

  • The code is very understandable and clear in what it needs to be comparing it to the other code.

Maintainability

  • As this is in the same style as the other code, it's quite maintainable being simple while doing the job.

Code Simplicity

  • This code is simple in it's execution following established design patterns and best practices .

Edge Cases

  • The bug fix deals and fixes an edge case

Test Thoroughness

  • Tested in mingw64 sktest and sk_unit_test and they both worked

Backward Compatibility

  • The code doesn't break existing functionality in the way it works.

Performance Considerations

  • Performance works well but it isn't usually a consideration for this type of change

Security Concerns

  • This code has no impact negatively or positively for security.

Dependencies

  • There are no new added dependencies.

Documentation

  • The documentation provided is through and simple to follow at the same time

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