Conversation
The existing test for ignoring PKL files when the binary is missing was not safe for parallel execution, as it modified a global function variable, creating a data race condition. This commit resolves the issue by refactoring the validation logic: 1. The responsibility for checking for the pkl binary is moved from the CLI into the `PklValidator`. 2. The validator now returns a specific `ErrSkipped` error if the binary is not found. 3. A thread-safe `SetPklBinaryChecker` function is introduced, allowing tests to safely mock the check. 4. The test is updated to use this new thread-safe mechanism.
- Removes a redeclared isPklBinaryPresent function that was causing a build failure. - Updates the test logic to check for the new ErrSkipped sentinel error, aligning it with the recent refactoring. - Removes an unused import.
- Fixes a build failure in the filetype package by using the correct `tools` import. - Updates the `good.pkl` test fixture with valid syntax so that tests pass when the pkl binary is installed.
kehoecj
added
the
waiting-on-maintainer-review
|
@akhil-rasheed Thanks for the PR! Please address the issues identified in the unit and mega linter jobs. |
kehoecj
added
OSS Community Contribution
Refactors the pkl validator to improve testability and adds tests for uncovered code paths. - Modifies the `PklValidator` to allow injecting an evaluator factory, making it easier to test error conditions. - Adds a test to verify that `ErrSkipped` is returned when the `pkl` binary is not found. - Adds a test to verify that an error is returned when the pkl evaluator fails to be created.
|
I've addressed the coverage issues and some whitespace in the workflow file which was causing the yaml linter to fail @kehoecj |
akhil-rasheed
force-pushed
the
feat/pkl-validation
branch
2 times, most recently
from
ade80f0 to
309847e
Compare
Still seeing at least one test failure: #331 (comment) |
Removes `t.Parallel()` from tests that modify the global `isPklBinaryPresent` variable. This prevents potential race conditions between tests that rely on the state of this variable.
Resolved |
Can you take another look? I'm still seeing a failure in the pipeline |
This is on the CI, I assume this is where it's failing, but the same test passes for me locally Am I just not identifying the failure correctly? |
That appears to be the correct failure - let me see if it passes locally for me as well. There may be a timing issue that is present in the pipeline causing it to fail |
|
I have made the changes that @ccoVeille requested. @kehoecj were you able to replicate the pipeline issue locally? |
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
kehoecj
left a comment
kehoecj
left a comment
There was a problem hiding this comment.
When the PKL executable is not installed, a runtime error is generated:
> .\validator.exe .\test\
Warning: 'pkl' binary not found, file C:\Users\se456c\src\github.com\prs\pkl\config-file-validator\test\fixtures\good.pkl will be ignored.
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0x7ff78ccb90f0]
goroutine 22 [running]:
github.com/apple/pkl-go/pkl.(*execEvaluator).deinit(0xc000206140)
C:/Users/se456c/go/pkg/mod/github.com/apple/pkl-go@v0.8.0/pkl/evaluator_manager_exec.go:214 +0x70
github.com/apple/pkl-go/pkl.(*evaluatorManager).closeErr(0xc000193bc0, {0x7ff78cee41c0?, 0xc000116e80?})
C:/Users/se456c/go/pkg/mod/github.com/apple/pkl-go@v0.8.0/pkl/evaluator_manager.go:308 +0x82
github.com/apple/pkl-go/pkl.(*evaluatorManager).listenForImplClose(0xc000193bc0)
C:/Users/se456c/go/pkg/mod/github.com/apple/pkl-go@v0.8.0/pkl/evaluator_manager.go:245 +0x46
created by github.com/apple/pkl-go/pkl.NewEvaluatorManagerWithCommand in goroutine 1
C:/Users/se456c/go/pkg/mod/github.com/apple/pkl-go@v0.8.0/pkl/evaluator_manager_exec.go:61 +0x256
Tests are failing with the latest changes due to the runtime error |
|
@akhil-rasheed Just checking in - any questions or concerns about the last set of review comments? |
|
All good, will restart work on this starting Monday |
Use the in-process pkl-go interpreter for validation instead of requiring the pkl binary to be installed. This simplifies usage by removing the external dependency.
|
@akhil-rasheed I'm working on a 1.10 release and would like to add your PKL changes. Your MR is close, just need to resolve some merge conflicts and fix the segfault in the unit tests |
|
Wasn't aware the pipeline had failed, will take a look tomorrow |
…klValidator.Validate to PklValidator.ValidateSyntax and added PklValidator.ValidateFormat to correctly implement the Validator interface. Updated test calls in validator_test.go accordingly
|
Hey @ccoVeille can you rerun the workflows again? Unable to reproduce the segfault locally, but I've merged from main and fixed some breaking changes |
|
I don't have the privilege to launch it unfortunately @akhil-rasheed I'm not member I thought I had 😅 |
|
@ccoVeille I'm not able to reproduce this segfault locally, the tests run just fine for me. Are there docs to replicate the test environment locally? |
|
@kehoecj might be able to help |
|
@akhil-rasheed I just pushed a major refactor to the unit tests to fix some of the flakiness - there have been a lot of changes since your initial PR. At this point, you may want to close this PR, get a clean copy of main, and then add your PKL changes |
4 participants
Closes #141
Addresses following comments left on the original (abandoned) PR here #164