-
Notifications
You must be signed in to change notification settings - Fork 28
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/self codes #156
Feature/self codes #156
Conversation
👋 A new build is available for this PR based on 40fa092. |
👋 A new build is available for this PR based on cbfe95e. |
Added testing framework for self codes. We can add more as we please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial reaction is good. I am currently looking for a system to do tests on (my 7.3 box won't cut it).
In the mean time, the first thing to change is any reference of "SELFCODES" in labels to be "SELF codes"/"SELF code".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review is that it works, but the organisation of the 'set codes' method needs to be part of the SQL job and not a seperate function.
👋 A new build is available for this PR based on ebcaf56. |
👋 A new build is available for this PR based on bd76b15. |
👋 A new build is available for this PR based on ebb847e. |
👋 A new build is available for this PR based on 7c3126c. |
👋 A new build is available for this PR based on 44ca294. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good - just a couple of minor package.json
changes, and perhaps some changing of the test cases is in order.
I actually think the SELF support should go into its own TestSuite
instead of being part of the manager. Also, only some of the test cases are passing. What am I doing wrong? I am on a a 7.4 system. (oss74dev
)
👋 A new build is available for this PR based on 82d0b51. |
👋 A new build is available for this PR based on dfd8f68. |
👋 A new build is available for this PR based on 4e9890a. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ajshedivy - the tests are passing! Let's merge.
Test Script: