Add tests for `computeSchemaChecksum` #3733

naveedkhan8067 · 2022-06-02T09:19:01Z

These new tests verifies the functionality of two new functions that generates the sha1 hash for schema xml's

Corresponding native PR.

core/backend/src/test/TestUtils.ts

pmconne

IModelHost.platform is @internal. You need to provide a public API in @itwin/core-backend that can be called from other packages to compute schema checksums.

naveedkhan8067 · 2022-06-07T10:37:45Z

Should I add these functions to an existing class like schema for example as they are related to schema XML's checksum? or create a new exported functionality?

pmconne · 2022-06-07T10:45:38Z

Should I add these functions to an existing class like schema

Probably makes sense to add static methods to the Schema class in core-backend.

naveedkhan8067 · 2022-06-07T11:01:10Z

Should I add these functions to an existing class like schema

Probably makes sense to add static methods to the Schema class in core-backend.

ok and in that static function definition, I can call IModelHost.platform.computeSchemaChecksum?

pmconne · 2022-06-07T11:02:33Z

in that static function definition, I can call IModelHost.platform.computeSchemaChecksum

Yep, that's the whole point.

naveedkhan8067 · 2022-06-07T13:38:41Z

in that static function definition, I can call IModelHost.platform.computeSchemaChecksum

Yep, that's the whole point.

When I add a static function into the Schema class and do build it is successful but while test I get TypeError: Class extends value undefined is not a constructor or null. You have any idea?

pmconne · 2022-06-07T13:42:26Z

TypeError: Class extends value undefined is not a constructor or null

Sounds like circular import. Push your changes so we can see. Also provide the full text of the error (with line numbers etc).

naveedkhan8067 · 2022-06-07T13:46:34Z

TypeError: Class extends value undefined is not a constructor or null

Sounds like circular import. Push your changes so we can see. Also provide the full text of the error (with line numbers etc).

Ok let me do that.

naveedkhan8067 · 2022-06-07T13:52:35Z

TypeError: Class extends value undefined is not a constructor or null

Sounds like circular import. Push your changes so we can see. Also provide the full text of the error (with line numbers etc).

Ok let me do that.
TypeError: Class extends value undefined is not a constructor or null at Object.<anonymous> (D:\SignoffTool\itwinjs-core\1\itwinjs-core\core\backend\src\BisCoreSchema.ts:35:36) at Module._compile (internal/modules/cjs/loader.js:1063:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10) at Module.load (internal/modules/cjs/loader.js:928:32) at Function.Module._load (internal/modules/cjs/loader.js:769:14) at Module.require (internal/modules/cjs/loader.js:952:19) at require (internal/modules/cjs/helpers.js:88:18) at Object.<anonymous> (D:\SignoffTool\itwinjs-core\1\itwinjs-core\core\backend\src\IModelHost.ts:18:1) ...52 lines omitted... at Function.Module._load (internal/modules/cjs/loader.js:769:14) at Module.require (internal/modules/cjs/loader.js:952:19) at require (internal/modules/cjs/helpers.js:88:18) at Object.exports.requireOrImport (D:\SignoffTool\itwinjs-core\1\itwinjs-core\common\temp\node_modules\.pnpm\mocha@8.4.0\node_modules\mocha\lib\esm-utils.js:42:12) at Object.exports.loadFilesAsync (D:\SignoffTool\itwinjs-core\1\itwinjs-core\common\temp\node_modules\.pnpm\mocha@8.4.0\node_modules\mocha\lib\esm-utils.js:55:34) at Mocha.loadFilesAsync (D:\SignoffTool\itwinjs-core\1\itwinjs-core\common\temp\node_modules\.pnpm\mocha@8.4.0\node_modules\mocha\lib\mocha.js:473:19) at singleRun (D:\SignoffTool\itwinjs-core\1\itwinjs-core\common\temp\node_modules\.pnpm\mocha@8.4.0\node_modules\mocha\lib\cli\run-helpers.js:125:15) at exports.runMocha (D:\SignoffTool\itwinjs-core\1\itwinjs-core\common\temp\node_modules\.pnpm\mocha@8.4.0\node_modules\mocha\lib\cli\run-helpers.js:190:10) at Object.exports.handler (D:\SignoffTool\itwinjs-core\1\itwinjs-core\common\temp\node_modules\.pnpm\mocha@8.4.0\node_modules\mocha\lib\cli\run.js:362:11) at D:\SignoffTool\itwinjs-core\1\itwinjs-core\common\temp\node_modules\.pnpm\yargs@16.2.0\node_modules\yargs\build\index.cjs:443:71

pmconne · 2022-06-07T14:07:23Z

Yes, it's a circular import. You added an import from IModelHost into Schema.ts.
Options:

Make a separate file containing a free function to compute the checksums; or
Make a separate file declaring namespace Schema containing function to compute the checksums; or
Spent all day separating things into new files, adding interfaces to eliminate dependencies, etc, and hope you get a configuration that removes the circular dependency.
???

kabentley · 2022-06-07T14:34:40Z

You could actually move your static method onto BisCoreSchema to avoid the circular dependency. Alternatively just make it a static method on IModelHost.

I frankly don't understand why there's two internal methods rather than one method that takes named arguments with an optional "exact" arg. Your new public method should use named arguments too.

…port issue

naveedkhan8067 · 2022-06-08T09:33:40Z

circular import

First option seems reasonable to me. Yesterday, I spent some time to resolve this circular import issue but I think it will be more time consuming.

naveedkhan8067 · 2022-06-08T09:40:16Z

You could actually move your static method onto BisCoreSchema to avoid the circular dependency. Alternatively just make it a static method on IModelHost.

I frankly don't understand why there's two internal methods rather than one method that takes named arguments with an optional "exact" arg. Your new public method should use named arguments too.

Yes, its seems possible but I think moving it there may represent it that its specific to BisCore. I can move it to IModelHost but that class don't seem related to it. @pmconne what you think?

pmconne · 2022-06-08T09:43:53Z

@pmconne what you think?

Fine by me. I will nitpick the name "getSchemaSha1Hash" since it's actually doing a fair amount of work (and on the main thread, but that's a separate conversation)...I would use "compute" or "generate" instead. But I won't argue over it.

…into io-merge-addon

naveedkhan8067 · 2022-06-08T09:54:47Z

@pmconne what you think?

Fine by me. I will nitpick the name "_get_SchemaSha1Hash" since it's actually doing a fair amount of work (and on the main thread, but that's a separate conversation)...I would use "compute" or "generate" instead. But I won't argue over it.

Yes, I think its a good suggestion. Let me update this name.

core/backend/src/ECSchemaOps.ts

core/backend/src/test/native/ECSchemaOps.test.ts

ColinKerr · 2022-06-09T12:41:43Z

IModelHost.platform is @internal. You need to provide a public API in @itwin/core-backend that can be called from other packages to compute schema checksums.

@pmconne This is an API that is just used to verify that schemas have not changed as part of our schema release process. I would prefer to have this be internal API with no long term contract on support. That way we can modify or remove it as is dictated by the needs of the schema release process. I thought this fit our definition of API that could be marked as @internal: We may call it outside the package it is defined in but we do not intend it to be called by 3rd parties and do not provide any guarantee API consistency between versions.

pmconne · 2022-06-09T12:51:41Z

@pmconne This is an API that is just used to verify that schemas have not changed as part of our schema release process.

By "public" I mean, exported from @itwin/core-backend. No package other than core-backend should be talking to IModelHost.platform.

kabentley · 2022-06-09T13:40:10Z

We may call it outside the package it is defined in but we do not intend it to be called by 3rd parties and do not provide any guarantee API consistency between versions.

Given this, i strongly suggest this be an (undocumented) @internal method on IModelHost:

@internal
public static computeSchemaChecksum(arg: { schemaXmlPath: string, referencePaths: string[], exactMatch?: boolean }) {
  return this.platform.computeSchemaChecksum(arg);
}

ColinKerr · 2022-06-09T13:51:19Z

We may call it outside the package it is defined in but we do not intend it to be called by 3rd parties and do not provide any guarantee API consistency between versions.

Given this, i strongly suggest this be an (undocumented) @internal method on IModelHost:
@internal
public static computeSchemaChecksum(arg: { schemaXmlPath: string, referencePaths: string[], exactMatch?: boolean }) {
  return this.platform.computeSchemaChecksum(arg);
}

Sounds like a plan!

core/backend/src/IModelHost.ts

core/backend/src/test/IModelHost.test.ts

kabentley

i think you need to run rush change before you finish this PR.

core/backend/src/test/TestUtils.ts

naveedkhan8067 · 2022-06-10T13:44:10Z

i think you need to run rush change before you finish this PR.

Ok

naveedkhan8067 · 2022-06-10T14:19:20Z

i think you need to run rush change before you finish this PR.

Ok

Pushed the file after rush change

* test native fixes for roundtripping placement when no geometry is present (#3642) * move up equalWithFpTolerance and add test for placement geometry json roundtrip * remove dead imports from development * rush change Co-authored-by: Michael Belousov <MichaelBelousov@users.noreply.github.com> * close transformer state dumps even if overrides threw an error (#3612) (#3656) * close transformer state dumps even if producing them threw an error Co-authored-by: Michael Belousov <MichaelBelousov@users.noreply.github.com> Co-authored-by: Michael Belousov <MichaelBelousov@users.noreply.github.com> * fix failing test due to (previous) rounding error in queryModelRange (#3675) * UI: Optimize creating subjects hierarchy in Models Tree (#3671) * Bump tile format version (#3732) * Add support for CloudSqlite (#3747) * work on cloud containers * make workspace containers use CloudSqlite * wip * workspace editor with new CloudSqlite * support CloudContainers for opening SQLiteDbs * add tests for CloudSqlite * test anonymous access * add tests for using CloudCache guid for write locks * work on download checkpoint * more download testing * allow creating and opening SQLiteDbs with params * cloud workspace tests * rework resolveContainerName * add WorkspaceDb versioning * fix tests * lint errors * enable checksums for block names * add makeVersion command to WorkspaceEditor * documentation cleanup * documentation cleanup * add --noPrompt for initializeWorkspace * support "@" scripts * fix package file * cleanup * don't allow writes to workspaceDbs unless they have been versioned in the current txn * fix tests * rename container.attach to container.connect * fix tests * add detach arg to disconnect * separate detach and disconnect * allow offline usage * collate nocase for listDb * add detach command to WorkpsaceEditor * show "friendly" file sizes * add version for testing cloud gcs * renames for customuri api * workspace settings * checkForChanges is not async * get tests working * load extra gcs databases stored in iModel settings * work on loading settings * work on settings * read all settings and schema files at startup * fix tests * old->newV2props, add test for daemon, start working on supporting v2checkpoint without daemon * doc cleanup * don't rename V2CheckpointAccessProps members * fixes, Nick please review * doc cleanup * wip * merge * fix tests * cleanup lint warnings * Add some tests, more changes around cache / blockcache_dir * doc * Add two env variables, prefetchbeforeopen, prefetchafteropen for testing. add non null assertions to some failing tests * wip * lint errors * fix tests * validate settings against schema * validate array entries * validate setting schemas * workspace documentation * afds * rename curl debugging option * wip * time prefetch * make editor functions async * workspaceEditor doc * document @ scripts * workspaceEditor doc * @ script examples * cleanup * new sqlite code for prefetch * prefetch testing * cleanup * add a PREFETCH_TIMe env variable * make WorkspaceDb.prefetch return the prefetch object * wip * merge * update @types/chai * add prefetch_nrequests * use parseSync for Yargs * log time taken for prefetch always * lint errors * allow choosing nRequests for prefetch testing * don't use env variables, switch to app settings to determine a prefetch * improve prefetch logging * lint errors * cleanup * allow importDb to supply extension for input file * use CloudContainer.blockSize * lint errors * use https for gcs data url Co-authored-by: nick.tessier <22119573+nick4598@users.noreply.github.com> * Fix requestElementGraphics (#3748) * Add tests for `computeSchemaChecksum` (#3733) * Functionality to generate sha1 hash for schema xml * consuming iModelHost * Adding static function to the schema class * Made the hash generation as separate functionality due to circular import issue * removed unnecessary import * function name updated with lint fixes * Moving changes to IModelHost * Documentation removed * combined test * line space added back * changelogs added * Presentation: Avoid counting target instances when calling getContentDescriptor RPC operation (#3816) * how about we avoid running rush update on imodel02 branch. * fix up core-backend * fix CheckpointManager test. * Delete core-transformer's HubMock Temporarily comment out TileCache tests. * unused imports. * fix TileCache.test.ts merge * extract-api; doc TODOs * lint * invalid doc links. * Fix presentation-backend test * remove unncessary dependencies from map-layers-auth * re-remove merge-restored removed deepEqualWithFpTolerance impl * Lock down ts-node to 10.8.0 for now because istanbuljs/nyc#1473 * fix slash direction * Lock superagent down to 7.1.3 because 7.1.6 has core-full-stack-test integration test failures in electron. * String.replaceAll no existe. * remove unnecessary pretest script that triggers an nyc bug * fix test errors * extract-api, lint * remove temporary test setting for gcs * fix lint error Co-authored-by: Michael Belousov <mike.belousov@bentley.com> Co-authored-by: Michael Belousov <MichaelBelousov@users.noreply.github.com> Co-authored-by: NancyMcCallB <45079789+NancyMcCallB@users.noreply.github.com> Co-authored-by: imodeljs-admin <38288322+imodeljs-admin@users.noreply.github.com> Co-authored-by: Robert Lukasonok <70327485+roluk@users.noreply.github.com> Co-authored-by: Caleb Shafer <31107829+calebmshafer@users.noreply.github.com> Co-authored-by: Arun George <aruniverse@users.noreply.github.com> Co-authored-by: johnnyd710 <19596966+johnnyd710@users.noreply.github.com> Co-authored-by: Bill Steinbock <65047615+bsteinbk@users.noreply.github.com> Co-authored-by: Mark Schlosser <47000437+markschlosseratbentley@users.noreply.github.com> Co-authored-by: Michel D'Astous <mdastous-bentley@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: bbastings <65233531+bbastings@users.noreply.github.com> Co-authored-by: Arun George <11051042+aruniverse@users.noreply.github.com> Co-authored-by: kckst8 <6283674+kckst8@users.noreply.github.com> Co-authored-by: AlainRobertAtBentley <73677355+AlainRobertAtBentley@users.noreply.github.com> Co-authored-by: Alina Paliulionytė <67429235+Alina657@users.noreply.github.com> Co-authored-by: Alina Paliulionyte <Alina657@users.noreply.github.com> Co-authored-by: Daniel Toby <41296254+DanielToby@users.noreply.github.com> Co-authored-by: kabentley <33296803+kabentley@users.noreply.github.com> Co-authored-by: Grigas <35135765+grigasp@users.noreply.github.com> Co-authored-by: nick.tessier <22119573+nick4598@users.noreply.github.com> Co-authored-by: naveedkhan8067 <38525837+naveedkhan8067@users.noreply.github.com>

Functionality to generate sha1 hash for schema xml

7da17f9

naveedkhan8067 requested a review from a team as a code owner June 2, 2022 09:19

naveedkhan8067 requested a review from ColinKerr June 2, 2022 11:39

ColinKerr reviewed Jun 3, 2022

View reviewed changes

core/backend/src/test/TestUtils.ts Outdated Show resolved Hide resolved

consuming iModelHost

16290ac

pmconne requested changes Jun 7, 2022

View reviewed changes

Adding static function to the schema class

95b71dc

Made the hash generation as separate functionality due to circular im…

e6ac456

…port issue

Merge branch 'imodel02' into io-merge-addon

c69351c

pmconne approved these changes Jun 8, 2022

View reviewed changes

naveedkhan8067 added 2 commits June 8, 2022 11:44

removed unnecessary import

2f2152e

Merge branch 'io-merge-addon' of https://github.com/iTwin/itwinjs-core …

5a07cd6

…into io-merge-addon

function name updated with lint fixes

159aa94

kabentley suggested changes Jun 9, 2022

View reviewed changes

core/backend/src/ECSchemaOps.ts Outdated Show resolved Hide resolved

kabentley reviewed Jun 9, 2022

View reviewed changes

core/backend/src/test/native/ECSchemaOps.test.ts Outdated Show resolved Hide resolved

Moving changes to IModelHost

90fe227

kabentley reviewed Jun 10, 2022

View reviewed changes

core/backend/src/IModelHost.ts Show resolved Hide resolved

core/backend/src/test/IModelHost.test.ts Outdated Show resolved Hide resolved

kabentley changed the title ~~ECSchemaOpsAddon functionality merged into IModelJsNodeAddon~~ Add tests for computeSchemaChecksum Jun 10, 2022

naveedkhan8067 added 2 commits June 10, 2022 15:11

Documentation removed

c57f2c4

combined test

9a21eb6

kabentley approved these changes Jun 10, 2022

View reviewed changes

core/backend/src/test/TestUtils.ts Outdated Show resolved Hide resolved

line space added back

a62a684

changelogs added

c3e6ef7

ColinKerr approved these changes Jun 10, 2022

View reviewed changes

naveedkhan8067 merged commit 82d342a into imodel02 Jun 10, 2022

naveedkhan8067 deleted the io-merge-addon branch June 10, 2022 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for `computeSchemaChecksum` #3733

Add tests for `computeSchemaChecksum` #3733

naveedkhan8067 commented Jun 2, 2022 •

edited by kabentley

Loading

pmconne left a comment

naveedkhan8067 commented Jun 7, 2022

pmconne commented Jun 7, 2022

naveedkhan8067 commented Jun 7, 2022

pmconne commented Jun 7, 2022

naveedkhan8067 commented Jun 7, 2022

pmconne commented Jun 7, 2022 •

edited

Loading

naveedkhan8067 commented Jun 7, 2022

naveedkhan8067 commented Jun 7, 2022

pmconne commented Jun 7, 2022 •

edited

Loading

kabentley commented Jun 7, 2022

naveedkhan8067 commented Jun 8, 2022

naveedkhan8067 commented Jun 8, 2022 •

edited

Loading

pmconne commented Jun 8, 2022

naveedkhan8067 commented Jun 8, 2022

ColinKerr commented Jun 9, 2022 •

edited

Loading

pmconne commented Jun 9, 2022

kabentley commented Jun 9, 2022 •

edited

Loading

ColinKerr commented Jun 9, 2022

kabentley left a comment

naveedkhan8067 commented Jun 10, 2022

naveedkhan8067 commented Jun 10, 2022

Add tests for computeSchemaChecksum #3733

Add tests for computeSchemaChecksum #3733

Conversation

naveedkhan8067 commented Jun 2, 2022 • edited by kabentley Loading

pmconne left a comment

Choose a reason for hiding this comment

naveedkhan8067 commented Jun 7, 2022

pmconne commented Jun 7, 2022

naveedkhan8067 commented Jun 7, 2022

pmconne commented Jun 7, 2022

naveedkhan8067 commented Jun 7, 2022

pmconne commented Jun 7, 2022 • edited Loading

naveedkhan8067 commented Jun 7, 2022

naveedkhan8067 commented Jun 7, 2022

pmconne commented Jun 7, 2022 • edited Loading

kabentley commented Jun 7, 2022

naveedkhan8067 commented Jun 8, 2022

naveedkhan8067 commented Jun 8, 2022 • edited Loading

pmconne commented Jun 8, 2022

naveedkhan8067 commented Jun 8, 2022

ColinKerr commented Jun 9, 2022 • edited Loading

pmconne commented Jun 9, 2022

kabentley commented Jun 9, 2022 • edited Loading

ColinKerr commented Jun 9, 2022

kabentley left a comment

Choose a reason for hiding this comment

naveedkhan8067 commented Jun 10, 2022

naveedkhan8067 commented Jun 10, 2022

Add tests for `computeSchemaChecksum` #3733

Add tests for `computeSchemaChecksum` #3733

naveedkhan8067 commented Jun 2, 2022 •

edited by kabentley

Loading

pmconne commented Jun 7, 2022 •

edited

Loading

pmconne commented Jun 7, 2022 •

edited

Loading

naveedkhan8067 commented Jun 8, 2022 •

edited

Loading

ColinKerr commented Jun 9, 2022 •

edited

Loading

kabentley commented Jun 9, 2022 •

edited

Loading