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

Add encoding tests for CCSID handling #2334

Merged
merged 131 commits into from
Nov 26, 2024
Merged

Conversation

worksofliam
Copy link
Contributor

@worksofliam worksofliam commented Nov 8, 2024

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:

  • Browsing the QSYS file system when libraries and source files have variants in the name
  • Opening members with variants in the name
  • Running ILE commands with variants in them
  • Running SQL statements with variants in the statement

Many test cases were worked on under the Encoding suite and they are all passing, showing that we can now work with variants.

Changes

  • Introduces new component, cqsh, which wraps _SETCCSID and qsh together since qsh always ignores user CCSID when launching from a non-interactive shell. We need to update our docs with info about this!
  • runSQL now uses cqsh when available as it solves issues with variant characters in the statement and in the result set
  • runSQL now determines if CSVs are needed based on the availability of cqsh and CCSID
  • runCommand and runSql can now take care of variants instead of letting the callers deal with it
  • Changes when variant characters are fetched (always towards the end)
  • getObjectList now using for bit data on source files
  • validateLibraryList now supports variant names
  • Fixes to when $ is escaped (also added generic function IBMi.escapeForShell)
  • Convert IBMi#dangerousVariants into getter instead of class property

Changes not related to the fix

  • Add additional parameter to runSQL to allow for pseudo parameters, which automatically take care of encoding the input to ensure it's passed the OS correctly
  • Added the ability to run test suites simultaneously & on a specific fixture
  • Fix test 'check getMemberInfo' which has been broken since we introduced components
  • Remove some invalid tests for testing getTable without enableSQL property
  • Improved the PR workflow to run every commit and not spam new comments for each build, but instead update an existing comment.
  • Ability to define different fixtures against the same system

Testing

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.

  • Solve 'Test downloadMemberContent with dollar' with Danish
  • Solve 'download' against a source file when variant char is used (this logic is embedded in the Object Browser)
  • Ensure tests are passing on PUB400.
  • Ensure tests are passing on 7.3
  • Ensure tests are passing on 7.4/7.5

Signed-off-by: worksofliam <[email protected]>
@worksofliam worksofliam linked an issue Nov 8, 2024 that may be closed by this pull request
Signed-off-by: worksofliam <[email protected]>
Signed-off-by: worksofliam <[email protected]>
@worksofliam worksofliam marked this pull request as ready for review November 9, 2024 16:41
@worksofliam
Copy link
Contributor Author

worksofliam commented Nov 10, 2024

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.

@worksofliam worksofliam requested a review from Copilot November 23, 2024 15:01

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]>
src/api/IBMiContent.ts Outdated Show resolved Hide resolved
src/testing/encoding.ts Show resolved Hide resolved
src/testing/encoding.ts Show resolved Hide resolved
src/api/IBMi.ts Outdated Show resolved Hide resolved
src/api/IBMi.ts Outdated Show resolved Hide resolved
src/api/IBMi.ts Outdated Show resolved Hide resolved
src/api/IBMi.ts Show resolved Hide resolved
src/api/IBMiContent.ts Show resolved Hide resolved
package.json Show resolved Hide resolved
src/api/IBMi.ts Outdated Show resolved Hide resolved
Copy link
Member

@SanjulaGanepola SanjulaGanepola left a 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 🎉

@worksofliam
Copy link
Contributor Author

@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 (# and $). Thanks!

@worksofliam worksofliam linked an issue Nov 25, 2024 that may be closed by this pull request
Copy link
Member

@SanjulaGanepola SanjulaGanepola left a 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.

@worksofliam worksofliam merged commit aa171eb into master Nov 26, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants