cap-js / cds-typer

CDS type generator for JavaScript
Apache License 2.0
29 stars 10 forks source link

Fixes cds.entities and entity name issues in proxy function #325

Closed stockbal closed 2 months ago

stockbal commented 2 months ago

I discovered some bugs in my proxy creator function.

  1. The fully qualified name will be wrong if an empty namespace is passed to the function
  2. for some reason the cds.entities[name] variant can only used if at least 2 CDS files are located in the /db folder.

Only runtime tests could discover those issues.

@daogrady Are there runtime tests in the project or is it planned to have them? For features such as the proxy function it would be great.

daogrady commented 2 months ago

Are there runtime tests in the project or is it planned to have them? For features such as the proxy function it would be great.

Kind of. We do have a new kind of tests that will effectively call cds.serve, which, I guess, should cover these cases. I have described them in the wiki.

daogrady commented 2 months ago

@stockbal would you like to add this kind of test cases to the PR and if so, do you need assistance in doing so? NB: the entire test part of cds-typer needs another overhaul anyway, as most tests that are currently declared as "unit tests" are actually functional tests and the kind you are describing should be categorised as integration tests, so I will soon restructure the tests.

stockbal commented 2 months ago

@daogrady I would like to add such tests. If they are to be part of this PR or an additional one depends mostly on when you plan to have this fix in a new version.

If a restructuring of the tests is imminent, I could also wait until it is done and write my tests then.

I imagine something like the following for the 2 bugs I discovered.

(async () => {
  const { Books } = require("./@cds-models/CatalogService");

  try {
    // this access should fail here, as the cds runtime is not yet loaded
    Books.elements;
  } catch (error) {
    console.error(error);
  }

  // start up cds runtime. I just used cds.test() in my small sample file
  await cds.test();

  try {
    Books.elements;
    console.log("Elements access successful");
  } catch (error) {
    // we should not run into this error after cds is up and running
    console.error(error);
  }

  const srv = await cds.connect.to("CatalogService");

  srv.prepend((srv) => {
      // the following could be convenient way to test that a handler registration is successful
    try {
      srv.after("READ", Books, (data) => {
        console.log("After.read.Books");
      });
      console.log("Event registered");
    } catch (error) {
      // we should not arrive here
    }
  });
})();

Of course the code has to be rewritten into jest test syntax. Apart from that, if this is going totally in the wrong direction, then some support from your side would be most welcome 😊.

Regards, Ludwig

daogrady commented 2 months ago

depends mostly on when you plan to have this fix in a new version.

You are the first and so far only user to report this problem, but it seems like a rather grave issue, so I'd say: rather sooner than later. 🙂

If a restructuring of the tests is imminent, I could also wait until it is done and write my tests then.

This probably won't happen before October, just wanted to let you know lest you wonder where your tests have gone once I restructure the directory. But for now, you can just include it in the old unit test directory, no problem.

I imagine something like the following for the 2 bugs I discovered.

That looks reasonable to me. Feel free to just add it in an appropriate form. 👍

stockbal commented 2 months ago

Hi Daniel,

the runtime tests are added.

Regards, Ludwig