Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore test.ts usages #8

Merged
merged 14 commits into from
Jan 16, 2024
Merged

Restore test.ts usages #8

merged 14 commits into from
Jan 16, 2024

Conversation

danjoa
Copy link
Contributor

@danjoa danjoa commented Dec 14, 2023

No description provided.

@danjoa danjoa requested a review from daogrady December 14, 2023 13:07
@daogrady
Copy link
Contributor

@danjoa I have fixed the import error that caused the test to fail before. We now have a new problem during runtime where log apparently does not behave as expected. Could you please take a look if the runtime behaviour is maybe off?

@danjoa
Copy link
Contributor Author

danjoa commented Dec 20, 2023

I see you added these lines 2 weeks ago. → these likely never worked, did they?
cds.test.log() is designed to work in tests and only in there.

@daogrady
Copy link
Contributor

Sorry, I am not entirely clear on that. As I have copied the contents of this repo over from our internal one I am showing up as author on most files right now. But the lines in question have been present in the internal repository since September, not authored by me. So I can not speak for whether they ever worked. But it sounds like we can just remove those, as they are not supposed to work?

Copy link
Member

@chgeo chgeo left a comment

Choose a reason for hiding this comment

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

I got the tests green finally.

@daogrady @danjoa we can discuss them together, like

  • Shall we keep this file as whole, which is bit of a mish mash, or shall we separate it to the different aspects?
  • The .drafts element is only available in old/non-lean draft mode. Why?

.github/workflows/test.yml Show resolved Hide resolved
apis/core.d.ts Outdated Show resolved Hide resolved
apis/linked.d.ts Outdated Show resolved Hide resolved
apis/models.d.ts Outdated Show resolved Hide resolved
test/typescript/runtime.test.ts Show resolved Hide resolved
test/typescript/runtime.test.ts Show resolved Hide resolved
test/typescript/runtime.test.ts Show resolved Hide resolved
test/typescript/tsconfig.json Show resolved Hide resolved
@chgeo
Copy link
Member

chgeo commented Jan 16, 2024

@daogrady can you check again, so that we can merge this?

Copy link
Contributor

@daogrady daogrady left a comment

Choose a reason for hiding this comment

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

Seeing that all tests pass, I think we are good to go. Thank you for your contributions!

@chgeo chgeo merged commit 2088601 into main Jan 16, 2024
4 checks passed
@chgeo chgeo deleted the restore-test.ts branch January 16, 2024 08:37
chgeo added a commit that referenced this pull request Jan 16, 2024
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