-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add encoding tests for CCSID handling #2334
Conversation
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
In theory, the new test case added that checks variants should be able to run no matter the QCCSID or runtime CCSID as it should work with all variants. |
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.
Copilot reviewed 14 out of 29 changed files in this pull request and generated no suggestions.
Files not reviewed (15)
- .vscode/launch.json: Language not supported
- package.json: Language not supported
- src/components/cqsh/cqsh.c: Language not supported
- .github/workflows/pr.yaml: Evaluated as low risk
- src/testing/content.ts: Evaluated as low risk
- src/testing/connection.ts: Evaluated as low risk
- src/filesystems/qsys/QSysFs.ts: Evaluated as low risk
- src/testing/deployTools.ts: Evaluated as low risk
- src/api/Storage.ts: Evaluated as low risk
- src/testing/action.ts: Evaluated as low risk
- src/extension.ts: Evaluated as low risk
- src/filesystems/qsys/extendedContent.ts: Evaluated as low risk
- src/api/CompileTools.ts: Evaluated as low risk
- src/components/manager.ts: Evaluated as low risk
- src/testing/encoding.ts: Evaluated as low risk
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
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.
This is an awesome PR that address soo many issues! Everything looks to be good (tested manually and with the test suite).
@worksofliam 10/10 with this one 🎉
Signed-off-by: worksofliam <[email protected]>
@SanjulaGanepola I need you to review one last time with my added commit. There are two new test cases to ensure streamfiles works with certain characters ( |
Signed-off-by: worksofliam <[email protected]>
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.
Looks good. New tests passed and confirmed it works manually as well.
Improved support for variant characters no matter the QCCSID. This has been impossible for us to test in the pass due to us not fetching the variants correctly before - which I have fixed in this PR - and now VS Code is correctly picking up the variants based on the user profile CCSID. This solves things like:
Many test cases were worked on under the Encoding suite and they are all passing, showing that we can now work with variants.
Changes
cqsh
, which wraps_SETCCSID
andqsh
together sinceqsh
always ignores user CCSID when launching from a non-interactive shell. We need to update our docs with info about this!runSQL
now usescqsh
when available as it solves issues with variant characters in the statement and in the result setrunSQL
now determines if CSVs are needed based on the availability ofcqsh
and CCSIDrunCommand
andrunSql
can now take care of variants instead of letting the callers deal with itgetObjectList
now usingfor bit data
on source filesvalidateLibraryList
now supports variant names$
is escaped (also added generic functionIBMi.escapeForShell
)IBMi#dangerousVariants
into getter instead of class propertyChanges not related to the fix
runSQL
to allow for pseudo parameters, which automatically take care of encoding the input to ensure it's passed the OS correctlygetTable
withoutenableSQL
propertyTesting
With the introduction with fixtures, we can now easily switch between consistent profiles on a connection. This PR has been tested against all fixtures (American, Danish, Spanish, French) and all tests are passing.