Replace goog.asserts and goog.testing.asserts #137
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closure assertions are being pruned due to the aggressiveness of the compiler's advanced optimizations. The test side is easily covered by using Jasmine's expectation system. For runtime asserts, we don't have a good replacement so we'll just copy the goog.asserts we need into a new namespace to prevent the Closure compiler from finding them.
Add a clang format guard to a particularly long, load bearing, comment we use to guide rewriting common js deps.
These were revealed to be broken by the clean test build environment. Need to remove more closure asserts.
Despite the comments, we do in fact rely on closure's COMPILED global in CommonJS tests. This variable signals some tests to skip checking data that has likely been optimized away and is used by things like debug_test.js that do name checking. Previously, this was exported by the (now dead) closure testing asserts export for CommonJS. It has now been re-homed to the testdeps.
lukesandberg
reviewed
Oct 5, 2022
| @@ -13,3 +13,9 @@ goog.require('goog.crypt.base64'); | |||
| goog.require('goog.testing.PropertyReplacer'); | |||
|
|
|||
| exports.goog = goog; | |||
|
|
|||
| // The COMPILED variable is set by Closure compiler to "true" when it compiles | |||
There was a problem hiding this comment.
why do we need this now?
There was a problem hiding this comment.
It's used in debug_test.js I believe to avoid doing dumping message names and trying to compare against something that has been optimized away.
The jspb.asserts namespace should be sufficient to prevent closure from pruning the asserts, so remove the redundant prefix from the function names.
* swap assert(foo instanceof bar) to assertInstanceof * swap many anonymous functions in tests to arrow functions * drop asserts export to commonjs since they're not needed in tests anymore * remove remaining goog.testing.TestCase require
lukesandberg
reviewed
Oct 6, 2022
asserts.js
Outdated
| * @return {string} A copy of `str` in which each occurrence of | ||
| * `%s` has been replaced an argument from `var_args`. | ||
| */ | ||
| function subs(pattern, subs) { |
There was a problem hiding this comment.
these top level variables will need to be 'scoped' somehow.
i would just do
jspb.asserts.subs = function(...)
and tag it as a @private
otherwise we are polluting the global scope
There was a problem hiding this comment.
Blah, sorry missed that and doassertfailure. Removed the former since it's dead code now that we're just using Error and fixed the latter.
There was a problem hiding this comment.
...and missed getType. Just got it now.
doAssertFailure was mistakenly glossed over. Now namespaced and set to private. subs is dead code, so just removed it.
lukesandberg
approved these changes
Oct 6, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closure assertions are being pruned due to the aggressiveness of the compiler's advanced optimizations. The test side is easily covered by using Jasmine's expectation system. For runtime asserts, we don't have a good replacement so we'll just copy the goog.asserts we need into a new namespace to prevent the Closure compiler from finding them.
Closes #136