[TS] Add generated content to repo to track future changes

[bjornharrtell]

• allows to run code generation and/or typescript tests without ending up with modified or untracked content
• harmonizes code generation check and typescript tests to generate the same content

[bjornharrtell]

Strange.. CI complains with:

Run scripts/check_generate_code.py
diff --git a/tests/optional-scalars/scalar-stuff.ts b/tests/optional-scalars/scalar-stuff.ts
index 6b70ded..a7[4](https://github.com/google/flatbuffers/runs/7566214476?check_suite_focus=true#step:4:5)8c22 100[6](https://github.com/google/flatbuffers/runs/7566214476?check_suite_focus=true#step:4:7)44
--- a/tests/optional-scalars/scalar-stuff.ts
+++ b/tests/optional-scalars/scalar-stuff.ts
@@ -20[7](https://github.com/google/flatbuffers/runs/7566214476?check_suite_focus=true#step:4:8),10 +207,6 @@ defaultEnum():OptionalByte {
   return offset ? this.bb!.readInt[8](https://github.com/google/flatbuffers/runs/7566214476?check_suite_focus=true#step:4:9)(this.bb_pos + offset) : OptionalByte.One;
 }
 
-static getFullyQualifiedName():string {
-  return 'optional_scalars.ScalarStuff';
-}
-

[bjornharrtell]

Looks like it was due to different gen options for tests vs ./scripts/generate_code.py. Hopefully fixed in additional commit.

[bjornharrtell]

This may look like alot of redundant content but I do believe it provides value to be able to see/detect future changes affecting generation. And AFAIK for other langs we already have the generated content in repo.

[aardappel]

If all is generated by #7161 and we are not sure what for, we should not be adding these to the repo.

We should only add to the repo files which are directly generated from the schemas we actually have test code written for, and which are shared with other languages.

[bjornharrtell]

@aardappel I agree and I cannot find any evidence of them actually being exercised in tests but I assume there is a sensible reason for why they were added, perhaps @jkuszmaul can clarify?

[jameskuszmaul-brt]

Those files are "tested" insofar as that the typescript compiler should be building them and type-checking and whatnot without generating errors. I didn't add any run-time logic exercising the modified files since it didn't feel too necessary for the code I was adding (since for the changes I was making, ensuring that the imports and types worked out was really the main concern). That's not to say that more tests wouldn't be good...

It wouldn't really be hard to make it codegen the files in a subdirectory, but as mentioned I think some other languages do the same thing at the top level of the tests/ directory, so I didn't consider it notable at the time.

Edit: Replying from the wrong account, but the same person as @jkuszmaul :)

[bjornharrtell]

Thanks @jameskuszmaul-brt, that makes sense to me.

[bjornharrtell]

@aardappel are you ok with review comment answers and could we merge?

[aardappel]

@bjornharrtell well none of what I asked about has been resolved, right? "that other PR added them and we don't know why" doesn't seem a good reason to add a ton of files in sub-optimal locations.. like I said before, I'd prefer it if we only merged files that are directly under test by our TS testing code, and I'd prefer it if any new files don't unnecessarily land in the root test dir.

Maybe I am wrong about some of this, but find it hard to ok this as-is without someone clearly stating that any of this is needed. I'll ping @dbaileychess

[dbaileychess]

@aardappel @bjornharrtell I'll take a look at this today.

[dbaileychess]

I agree with @aardappel, we should first figure out/clean up what files are being generated for TS and remove the ones that aren't being directly tested or at least not namespaced.

[jkuszmaul]

I feel like there's a disconnect here. As I noted above, the *.ts files get exercised (directly or indirectly via imports) by being compiled by the typescript compiler in tests/TypeScriptTest.sh. This then generates .js files adjacent to the .ts files (which could be changed)--the .js files are not being exercised. Running tsc alone exercises testing keyword clashes in the codegen, as well as any incorrect import paths, and goes a fair ways towards confirming correct codegen.

[dbaileychess]

@jkuszmaul Yeah, I understand that part of the test, but the actual output files are of no clear use and shouldn't be checked in. Can we write a test that tsc correctly works and returns OK, and just disregard the output files?

[bjornharrtell]

I also feel there is a disconnect. I agree it's unfortunate that some generated end up in the tests root but it's the same for other langs. This only rectifies the incomplete state that when running code generation you otherwise get multiple untracked files in the tree and it's not clear why they are not added as the rest is.

[dbaileychess]

Then the test script should delete those files explicitly after verifying tsc worked.

[bjornharrtell]

That could make sense. But my argument is that we do not primarily commit generated content for the purpose of runtime tests - for that purpose all it could in fact all be deleted after the test run. We commit it to track effects of code generation logic changes.

[dbaileychess]

I think it is a balance, and we do a poor job of it now. We dupe so much stuff in all of our generated code, that at this point just becomes noise. How many times do we need to see the generated output of an integer field for a given language? We probably have 100s of those in all of our generated files.

Ideally we would have a golden/ directory that just had generated output for each language and each feature, neatly namespaced/directorized. Then the test/ directory can just focus on tests that use the generated code. We kind of conflate the two.

[bjornharrtell]

I agree it is a bit messy and redundant but it is what it is. It's it not becoming less messy to make an exception for a subset of from TS generated source because it happens to not be exercised by test cases at this time. I would rather argue that it would make sense to add some trivial runtime tests for it bur why make that a requirement at this time?

[aardappel]

When I said "used by the tests", I mean there is human written code calling the functions in it. The fact that it gets "compiled" is in theory useful, but it is not necessary.

Other languages are not creating files to this extent (and certainly not in the root dir), so I don't see why we should make things much worse now. I'd prefer this to be cleaned up to the minimum files actually needed.

[bjornharrtell]

1. It's difficult (IMO) to determine "the minimum files actually needed". Also IMO it would make much more sense to say either no generated sources in repo or all of them. I strongly feel that being selective about it is problematic.
2. As for other langs not pulluting the root I just don't see it.. what about:

• arrays_test_generated.h
• monster_extra_generated.h
• monster_test_bfbs_generated.h
• monster_extra_my_game_generated.dart
• monster_test_generated.grpc.fb.cc
• monster_test_generated.grpc.fb.h
• monster_test_generated.h
• monster_test_generated.lobster
• monster_test_generated.py
• it goes on...

[bjornharrtell]

And yet another argument is that the generated code in question is of interest to reference and discuss in relation to https://github.com/google/flatbuffers/issues/7395 which I'm currently still a bit confused about if there is a bug or if it is per design and sensible.

[aardappel]

As I already explained before, a file like monster_test_generated.h is not the same thing, as it contains a whole dir worth of type definitions, unlikely TS which generates one file per type, thus should sit in a namespaced dir.

Regardless of what there already is, we should not make things worse, and this PR seems to go much beyond what we have so far in the wrong direction.

I am not sure why it is hard to determine what is needed, this PR is pulling in files which a) are not directly tested by test code and b) there's no equivalent in (most) other languages. It should be possible to figure out how to not generate them and/or not add them to this PR, and/or namespace things, surely.

I get the sense we're arguing what is easiest, not what is good for the project.

[dbaileychess]

Let's pause this PR until we clean up what tests/TypeScriptTest.sh outputs. We can do that in a separate PR. I would just have the generated code sent to a temp directory that is deleted at the conclusion.

[bjornharrtell]

I'm sorry that we are in such disagreement here.

[bjornharrtell]

Hmm, @dbaileychess and @aardappel what about if we create a dedicated folder structure (and/or subfolder in tests, I would suggest one per language) intended for and only for generated code? I could try to make that happen here for TS/JS if that is an acceptable middle ground.

[dbaileychess]

I think this will be not needed if we move our lang-specific files to their own directories. At that point, we can check in the files?

[bjornharrtell]

I think this will be not needed if we move our lang-specific files to their own directories. At that point, we can check in the files?

Agree. And as I see it doing that is actually resolving this issue.

[dbaileychess]

@bjornharrtell Can this be dropped?

[bjornharrtell]

@dbaileychess yep, closing.