Skip to content

Conversation

@maribethb
Copy link
Contributor

@maribethb maribethb commented May 14, 2025

My turn to do #2523

  • Disables the tests for shadow-block-converter as there's some jsdom problem with the test setup
  • updates package-lock for shadow-block-converter as at some point it was committed with incorrect changes
  • reverts 96b299b now that Blockly.Field.NBSP exists again
  • merges in master

@maribethb maribethb requested a review from a team as a code owner May 14, 2025 23:40
@maribethb maribethb requested review from gonfunko and removed request for a team May 14, 2025 23:40
@maribethb
Copy link
Contributor Author

maribethb commented May 14, 2025

this isn't ready for review yet, idk how to make it a draft after the fact

edit: ready now if tests pass

@maribethb maribethb changed the base branch from master to rc/v12.0.0 May 14, 2025 23:57
@maribethb maribethb requested a review from BenHenning May 15, 2025 00:01
Copy link
Collaborator

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @maribethb! Just a few small comments, but approving since CI is green and this generally looks good.

# failed after a package had its version updated on github but before
# that version was published to npm.

name: publish unpublished plugins
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
name: publish unpublished plugins
name: Publish unpublished plugins

For consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done from merging in master, I don't want to muddy up actual changes into what should be a merge commit. Anyway, the publish action was already lower case so it's not really any more inconsistent than publish already is

* @param obj The object to test.
* @returns True iff the object conforms to ImageProperties.
*/
function isImageProperties(obj: any): obj is ImageProperties {
Copy link
Collaborator

@BenHenning BenHenning May 15, 2025

Choose a reason for hiding this comment

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

Perhaps this would be worth adding to Core proper in the future. Worth filing an issue?

const assert = chai.assert;

suite('shadowBlockConversionChangeListener', function () {
suite.skip('shadowBlockConversionChangeListener', function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we perhaps file a TODO to re-enable these and add a line comment here pointing to that issue?

Or, do these pass now that the package-lock.json file has been corrected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #2525

@maribethb maribethb merged commit b8d3b55 into RaspberryPiFoundation:rc/v12.0.0 May 15, 2025
9 checks passed
BenHenning added a commit that referenced this pull request May 15, 2025
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/samples#making_and_verifying_a_change)

## The details
### Resolves

Fixes #2525

### Proposed Changes

This re-enables the shadow block converter test disabled in #2524.

### Reason for Changes

It's preferable to not keep tests disabled as it's easy to forget about them (and having them disabled increases the risk of discovering regressions being delayed or never noticed).

This particular fix is discussed in #2528. It's a bit complicated exactly how the global namespace is managed via `jsdom-global`, but essentially `SVGElement` (made available to Node.js via `jsdom`) is not being automatically bound to the global namespace and this works around the issue.

### Test Coverage

N/A -- This is a test-only change.

### Documentation

No documentation changes are needed.

### Additional Information

This is a slightly ugly workaround, but it seems viable for the medium-term while #2528 is considered.
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.

4 participants