cli icon indicating copy to clipboard operation
cli copied to clipboard

test: replicate incorrect bundling and missing assertions part of #1323

Open francocm opened this issue 1 year ago • 20 comments

Description

  • Fixes missing assertion on spec to be true in bundle.test.ts. Existing tests also failing.
  • Replicates the problem described in #1323

Related issue(s)

Replicates #1323

francocm avatar Apr 24 '24 11:04 francocm

I've pulled and synced the changes from master branch into here, and the good news is after #1415 (@aeworxet), the original issue mentioned in #1323 seems solved.

However, the corrected tests are now showing 1 new issue coming probably from the bundler.

The bundler, since 0.5.0, makes it clear that objects must be dereferenced to the maximum extent possible. At least that's how I'm interpreting this.

However, when running the tests, for ASyncAPI v3.0.0 docs, it's leaving the following behind:

components:
  messages:
    TestMessage:
      payload:
        type: string
      x-origin: '#/components/messages/TestMessage'

This happens for both x-origin use and without. In case of x-origin, it does get referenced through x-origin itself, but I believe this shouldn't matter. In fact, with x-origin: false, then there's no references at all, yet still present. The content gets dereferenced successfully, just not cleaned up.

Right now this is the only thing keeping it failing.

@peter-rr @Amzani @Souvikns @Shurtu-gal any thoughts how you'd like to proceed?

Options I can think of:

  1. Raise it as a separate bug, and for now I remove the 'offending' part from the test comparison, and this gets solved.
  2. It's not a bug (please verify my interpretation), so will simply fix the test and this gets solved.
  3. Get it resolved in bundler and keep this PR waiting until that gets fixed.

Thanks!

francocm avatar May 09 '24 15:05 francocm

@francocm I am afraid this is not an issue of Bundler, because Bundler doesn't have such complex logic that could lead to such errors. JSON schema is simply transferred to @apidevtools/json-schema-ref-parser with several conditions set, so this issue is most probably the doing of that package.

I have traced the issue up to the crawl() function in @apidevtools/json-schema-ref-parser and opened an issue in its repository.

aeworxet avatar May 13 '24 00:05 aeworxet

@aeworxet

Thanks for the analysis.

Worth adding that although this component is ignored, when x-origin: true is used, it's still being updated, as in that scenario, the x-origin: '#/components/messages/TestMessage' gets added, pointing to itself.

I have updated my PR to consider this outcome as expected in the tests, since the scope of this PR is on the issue raised in #1323 and:

  1. assertions that replicated the original problem
  2. assertions that were not actually being checked on existing tests
  3. will now act as better regression test library for future changes

Probably anything else needs to be done in a separate place, and could be tracked in a new issue + linked to the https://github.com/APIDevTools/json-schema-ref-parser/issues/349 you raised.

@peter-rr I've updated the PR with feedback, tests should be passing, and it's no longer a draft. Kindly let me know your thoughts at this stage.

Thanks 🙂

francocm avatar May 13 '24 12:05 francocm

/update

peter-rr avatar May 14 '24 17:05 peter-rr

Probably anything else needs to be done in a separate place, and could be tracked in a new issue + linked to the APIDevTools/json-schema-ref-parser#349 you raised.

Yeah, I agree with this 👍 Once the new tests are passing I think we could proceed to close the bug related https://github.com/asyncapi/cli/issues/1323

peter-rr avatar May 14 '24 17:05 peter-rr

/update

peter-rr avatar May 15 '24 09:05 peter-rr

Hi @peter-rr,

Thanks for following up. I've run npm run lint:fix and pushed it back.

Running npm run test seems to be running fine here?

  129 passing (45s)

Full log:

➜  asyncapi-cli git:(fix/1323/bundleNotBundlingExternalFiles) ✗ npm run test    
> @asyncapi/[email protected] pretest
> npm run build


> @asyncapi/[email protected] build
> rimraf lib && node scripts/fetch-asyncapi-example.js && tsc && oclif manifest && echo "Build Completed"

Fetched ZIP file
Unzipped all examples from zip
 ›   Warning: oclif update available from 4.5.0 to 4.8.8.
wrote manifest to /Users/<username_redacted>/workspace/asyncapi-cli/oclif.manifest.json
Build Completed

> @asyncapi/[email protected] test
> npm run test:unit


> @asyncapi/[email protected] test:unit
> cross-env NODE_ENV=development TEST=1 CUSTOM_CONTEXT_FILENAME="test.asyncapi-cli" CUSTOM_CONTEXT_FILE_LOCATION="" nyc --extension .ts mocha --require ts-node/register --require test/helpers/init.js --reporter spec --timeout 100000 "test/**/*.test.ts"



  bundle
    ✔ should throw error message if the file path is wrong

  bundle spec v3

  config:analytics
    with disable flag
    with enable flag
    with no flags
    with status flag

  config
    config:versions

  config:context, positive scenario
    config:context:current
    config:context:list
    config:context:add
    config:context:add
    config:context:edit
    config:context:use
    config:context:remove
    config:context:init
    config:context:init
    config:context:init
    config:context:init

  config:context, negative scenario
    config:context:add
    config:context:add
    config:context:add
    config:context:add

  config:context, negative scenario
    config:context:list

  convert
    with file paths
    with no arguments
    with no context file
    with target-version flag
    with output flag

  diff
    should handle AsyncAPI v3 document correctly
    with file paths, and there are no difference between the files
    yaml output: with file paths, and there are no difference between the files
    with file paths, and getting all changes
    with file paths, and getting breaking changes
    with file paths, and getting non-breaking changes
    with file paths, and getting unclassified changes
    with file paths, and getting all changes, passing flag
    YAML output, getting all changes
    should show error on breaking changes
    Markdown output with subtype as json, getting all changes
    Markdown output with subtype as yaml, getting all changes
    Other output with markdownSubtype flag provided, check for warning
    with logging diagnostics
    with custom standard file
    should throw error for invalid override path
    should throw error for invalid override json

  template
    should handle AsyncAPI v3 document correctly
    git clash
Generation failed
      ✔ should throw error if output folder is in a git repository (205ms)
    custom params
    disable-hooks
    debug
    no-overwrite
    should install template
Generator Error: Installation failed
    map-base-url

  models
    should handle AsyncAPI v3 document correctly
    for TypeScript
    for JavaScript
    for Python
    for Rust
    for C#
    for C++
    for Java
    for Go
    for Kotlin
    for Dart
    for PHP
    with logging diagnostics

  new
    create new file
    when asyncapi file already exists

  new glee
    creation of new project is successful
    when new project name already exists

  optimize
    no optimization needed
    with no arguments
    with no context file
    no-tty flag
    interactive terminal
    error if the asyncapi file is invalid

  validate
    with file paths
    with context names
    with no arguments
    with no context file
    with --log-diagnostics flag
    with --diagnostics-format flag
    with --fail-severity flag


  129 passing (45s)

-----------------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------
File                         | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                                                                            
-----------------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------
All files                    |   59.91 |     40.2 |   57.22 |   60.09 |                                                                                              
 lib                         |    79.7 |    51.54 |    87.5 |    79.5 |                                                                                              
  base.js                    |    83.9 |    56.75 |     100 |    83.9 | 63-64,94-95,132,140-153                                                                      
  flags.js                   |     100 |      100 |     100 |     100 |                                                                                              
  globals.js                 |    38.7 |        0 |       0 |    38.7 | 13-15,20-45                                                                                  
  parser.js                  |   89.74 |    60.97 |     100 |   89.47 | 96-98,100-102,121,124                                                                        
 lib/commands                |    90.9 |    73.71 |     100 |   90.82 |                                                                                              
  bundle.js                  |   86.48 |    55.55 |     100 |   86.48 | 26-30,57-58                                                                                  
  convert.js                 |   94.87 |    77.77 |     100 |   94.87 | 54,60                                                                                        
  diff.js                    |   88.49 |    78.33 |     100 |   88.49 | 49,71,90,107,117,138,182-184,186,194,229,256                                                 
  optimize.js                |   92.37 |     73.4 |     100 |   92.17 | 87,94-104,113                                                                                
  validate.js                |   95.65 |       75 |     100 |   95.65 | 18                                                                                           
 lib/commands/config         |   86.88 |    76.47 |     100 |   86.44 |                                                                                              
  analytics.js               |   80.55 |    69.23 |     100 |   80.55 | 35-48                                                                                        
  versions.js                |      96 |      100 |     100 |   95.65 | 26                                                                                           
 lib/commands/config/context |   75.64 |    17.77 |     100 |   75.64 |                                                                                              
  add.js                     |      84 |       50 |     100 |      84 | 19-20,25-26                                                                                  
  current.js                 |   58.33 |     7.69 |     100 |   58.33 | 16-29                                                                                        
  edit.js                    |   70.83 |        0 |     100 |   70.83 | 19-27                                                                                        
  init.js                    |     100 |      100 |     100 |     100 |                                                                                              
  list.js                    |   86.36 |       50 |     100 |   86.36 | 14-15,28                                                                                     
  remove.js                  |   69.56 |        0 |     100 |   69.56 | 18-26                                                                                        
  use.js                     |   69.56 |        0 |     100 |   69.56 | 18-26                                                                                        
 lib/commands/generate       |   67.05 |    57.79 |   58.13 |   67.15 |                                                                                              
  fromTemplate.js            |   59.67 |    44.73 |      60 |   59.56 | 39,49-68,119-120,130-139,145,148,155-163,169,187-189,195-196,207,222,225,241,247,258,277-339 
  models.js                  |   75.79 |    70.51 |   53.84 |   76.12 | 37-41,49-51,59,68,170,173,176,214,235-273                                                    
 lib/commands/new            |   49.47 |    20.77 |   45.83 |   49.47 |                                                                                              
  file.js                    |   55.17 |    24.48 |   66.66 |   55.17 | 33,39-113,122,130-131,143                                                                    
  glee.js                    |   42.57 |    14.28 |   33.33 |   42.42 | 39-117,130,133-134,144-152,163                                                               
  index.js                   |     100 |      100 |     100 |     100 |                                                                                              
 lib/errors                  |   65.81 |    31.25 |   65.21 |   66.07 |                                                                                              
  context-error.js           |   83.78 |      100 |   69.23 |    87.5 | 60-61,67-68                                                                                  
  diff-error.js              |     100 |      100 |     100 |     100 |                                                                                              
  generator-error.js         |     100 |      100 |     100 |     100 |                                                                                              
  specification-file.js      |   54.83 |    58.33 |      25 |   54.83 | 7-18,25-26,41-42,45-46,49-50                                                                 
  validation-error.js        |   31.03 |       15 |      50 |   31.03 | 8,14,19-48                                                                                   
 lib/models                  |   69.66 |     60.9 |   70.51 |    69.6 |                                                                                              
  Context.js                 |   80.28 |    61.66 |     100 |   80.14 | 74-75,88,107,133,136,150,164,167,182,207,227,245-267,291-294                                 
  SpecificationFile.js       |   82.64 |    72.13 |   84.37 |   82.35 | 43,56,75,86-90,99-106,117,120,123,140,143,159,188-193,220                                    
  Studio.js                  |   25.71 |        0 |       0 |   26.08 | 20-101,106-123                                                                               
 lib/utils                   |   12.67 |        0 |       0 |   13.04 |                                                                                              
  generator.js               |   12.67 |        0 |       0 |   13.04 | 11-13,19-149                                                                                 
 src                         |   14.92 |        0 |       0 |   15.15 |                                                                                              
  base.ts                    |   14.92 |        0 |       0 |   15.15 | 20-132                                                                                       
 src/commands                |   21.42 |     4.25 |   22.22 |   22.01 |                                                                                              
  optimize.ts                |   21.42 |     4.25 |   22.22 |   22.01 | 27,49-210                                                                                    
 src/errors                  |   20.87 |        0 |       0 |   20.87 |                                                                                              
  context-error.ts           |      40 |      100 |       0 |      40 | 11,13,15,17,19-24,30-31,37-38,44-45,51-52,58-59,65-66,72-73                                  
  specification-file.ts      |   13.79 |        0 |       0 |   13.79 | 4-5,11-15,22-23,30-54                                                                        
  validation-error.ts        |     3.7 |        0 |       0 |     3.7 | 11-57                                                                                        
 src/models                  |   23.82 |     7.43 |   10.52 |   24.22 |                                                                                              
  Context.ts                 |   27.77 |       15 |   13.33 |    27.2 | 85-121,125-139,143-153,157-170,174-182,189-200,208-215,223-260,280-309,335-340,351-358,368   
  SpecificationFile.ts       |   19.26 |        0 |    8.69 |   20.58 | 27-97,113,125-155,160-171,176-180,185-223                                                    
-----------------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------

> @asyncapi/[email protected] posttest
> rimraf ./test.asyncapi-cli

➜  asyncapi-cli git:(fix/1323/bundleNotBundlingExternalFiles) ✗ 

francocm avatar May 15 '24 09:05 francocm

Hello, @francocm! 👋🏼

    I'm 🧞🧞🧞 Genie 🧞🧞🧞 from the magic lamp. Looks like somebody needs a hand!

    At the moment the following comments are supported in pull requests:

    - `/please-take-a-look` or `/ptal` - This comment will add a comment to the PR asking for attention from the reviewrs who have not reviewed the PR yet.
    - `/ready-to-merge` or `/rtm` - This comment will trigger automerge of PR in case all required checks are green, approvals in place and do-not-merge label is not added
    - `/do-not-merge` or `/dnm` - This comment will block automerging even if all conditions are met and ready-to-merge label is added
    - `/autoupdate` or `/au` - This comment will add `autoupdate` label to the PR and keeps your PR up-to-date to the target branch's future changes. Unless there is a merge conflict or it is a draft PR. (Currently only works for upstream branches.)
    - `/update` or `/u` - This comment will update the PR with the latest changes from the target branch. Unless there is a merge conflict or it is a draft PR. NOTE: this only updates the PR once, so if you need to update again, you need to call the command again.

asyncapi-bot avatar May 15 '24 09:05 asyncapi-bot

Just realised, if it helps I'm using Node 20:

➜  asyncapi-cli git:(fix/1323/bundleNotBundlingExternalFiles) ✗ node --version
v20.12.2
➜  asyncapi-cli git:(fix/1323/bundleNotBundlingExternalFiles) ✗ npm --version 
10.5.0
➜  asyncapi-cli git:(fix/1323/bundleNotBundlingExternalFiles) ✗ 

francocm avatar May 15 '24 09:05 francocm

Just realised, if it helps I'm using Node 20

Thanks @francocm for the tip 👍 I've tried with v20 but still getting the errors. Also now I'm getting many errors during the building process after updating CLI to the latest version 1.14.0. Even working on master branch. Let me investigate why I'm getting those results 🤔 and I'll get back to you.

peter-rr avatar May 15 '24 10:05 peter-rr

@peter-rr Can you please try cloning, npm-installing, and running your tests from zero? Sometimes there are manual changes you completely forgot you made, and they are not displayed with git status because files are in .gitignore. Knowing the logic of Bundler, it explicitly compares the integer part of the asyncapi property of the second AsyncAPI Document with the integer part of the asyncapi property of the first AsyncAPI Document, and it is very hard to compare two integers incorrectly if no changes are introduced somewhere else in the code.

aeworxet avatar May 15 '24 11:05 aeworxet

Thanks @aeworxet 😄 I just needed npm-installing to include the latest version of @oclif/core dependency. Everything working fine now.

peter-rr avatar May 16 '24 16:05 peter-rr

@francocm LGTM 👍 All tests passing after the latest changes applied.

Now this PR needs to be reviewed by the codeowners before being merged.

peter-rr avatar May 16 '24 16:05 peter-rr

/ptal

peter-rr avatar May 16 '24 16:05 peter-rr

@Amzani @Souvikns @Shurtu-gal Please take a look at this PR. Thanks! :wave:

asyncapi-bot avatar May 16 '24 16:05 asyncapi-bot

/update

peter-rr avatar May 16 '24 16:05 peter-rr

/update

Shurtu-gal avatar May 17 '24 14:05 Shurtu-gal

This pull request has been automatically marked as stale because it has not had recent activity :sleeping:

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience :heart:

github-actions[bot] avatar Sep 15 '24 00:09 github-actions[bot]

still relevant

@francocm are you still working on this?

Shurtu-gal avatar Sep 16 '24 16:09 Shurtu-gal

Closing this ref: https://github.com/asyncapi/cli/issues/1323#issuecomment-2579882693

Shurtu-gal avatar Jan 09 '25 14:01 Shurtu-gal

UPDATE https://github.com/APIDevTools/json-schema-ref-parser/issues/349#issuecomment-2628833419

aeworxet avatar Feb 01 '25 07:02 aeworxet