#hello folks, when running tests for PR

1 messages · Page 1 of 1 (latest)

novel moon
#

how do you feel about making the behavior of JSON consistent and the possibility of updating the testcases to reflect that?

#

to reproduce the error, we can checkout the PR branch, and run the following command:

dagger call -m dev --source=.:default test custom --pkg=./core/integration --run=^TestModule/TestTypescriptInit -vvv

pale dock
#

@pallid bison 👆

novel moon
#

Hi @pallid bison, apologies for tagging you directly, but I know you are going to be on vacation soon. if you have any inputs for this, kindly let me know.

FWIW, once I updated the test to use hasPkgJSON instead of hasPkgJson, the test passes (and the PR CI is green). The testcase itself is not specific to verifying the behavior with string JSON, it just happened to have one.

pallid bison
novel moon
#

that is helpful. so maybe we can configure that acronym from within the go sdk instead of configuring it in core?

pallid bison
#

Yes. Needs to be accessible from Go's codegen.

novel moon
#

got it. let me try that. thank you so much for the pointer

novel moon
#

is there a way to conditionally initialize this on core side (e.g. identify that requested sdk is go, and then initialize it?)

#

on codegen side, it does not seem to be making a difference

novel moon
# 
$ cat json.diff 
diff --git a/core/json.go b/core/json.go
index 7541af5c4..3aabcbb46 100644
--- a/core/json.go
+++ b/core/json.go
@@ -8,15 +8,10 @@ import (
 
        "github.com/dagger/dagger/dagql"
        "github.com/dagger/dagger/dagql/call"
-       "github.com/dagger/dagger/dagql/strcase"
 )
 
 type JSON json.RawMessage
 
-func init() {
-       strcase.ConfigureAcronym("JSON")
-}
-
 func (p JSON) Bytes() []byte {
        return p
 }
diff --git a/core/schema/module.go b/core/schema/module.go
index 29c9a4bf8..64ce2065a 100644
--- a/core/schema/module.go
+++ b/core/schema/module.go
@@ -12,6 +12,7 @@ import (
        "github.com/dagger/dagger/core"
        "github.com/dagger/dagger/core/modules"
        "github.com/dagger/dagger/dagql"
+       "github.com/dagger/dagger/dagql/strcase"
        "github.com/dagger/dagger/engine"
 )
 
@@ -704,6 +705,9 @@ func (s *moduleSchema) moduleInitialize(
        if inst.Self.NameField == "" || inst.Self.SDKConfig == "" {
                return nil, fmt.Errorf("module name and SDK must be set")
        }
+       if inst.Self.SDKConfig == "go" {
+               strcase.ConfigureAcronym("JSON")
+       }
        mod, err := inst.Self.Initialize(ctx, inst.ID(), dagql.CurrentID(ctx))
        if err != nil {
                return nil, fmt.Errorf("failed to initialize module: %w", err)

the above patch fixes the issue. This needs to happen during module - Initialization on the engine. as otherwise query fails to find definition for type with right casing.

#

I will spend some time to understand why it triggered failure after replacing strcase library. it does seem like a "race condition" (if I may), on when this strcase lib gets loaded.

#

but otherwise how do you feel about adding a func configureAcronymForSDK(sdk string) to core, which can then configure language specific acronyms (if needed)

#

also cc @fleet rune to collect his opinion/feedback on this

fleet rune
fleet rune
fleet rune
severe widget
#

(catching up, still loading context) - I really don't think we can/should add per-language conditionals in the core code paths. In principle someone could be using a custom SDK called banana that is a Go SDK. I also see @pallid bison's concern about different languages having different conventions, but I suppose we at least need to make sure that all the types in the GraphQL schema are following the same convention, regardless of what language they came from. So if I define a FooJson type in Python I'd expect that to become FooJSON in GraphQL. Is it possible to do that?

fleet rune
#

which takes us to the flake that @novel moon was observing above where sometimes the GraphQL API was correctly generated and the test failed, and sometimes it didn't

severe widget
fleet rune
severe widget
#

it was originally added because tests failed without it; it wasn't possible to have a field of type dagger.JSON. But going back in the repo history I also found other times where we configured acronyms like that

#

it sounds like GraphQL doesn't really have a convention around this either

pallid bison
severe widget
#

I guess the question is what configuring an acronym does: does it merely say "preserve JSON when you see it", or does it also say "convert Json to JSON"?

#

oh maybe you can ConfigureAcronym("Json", "JSON")?

pallid bison
#

Yeah, acronyms is a convention in Go, not JavaScript or GraphQL. I'd do away with that complication if we could, but it's expected in Go. You need to manage a list with the supported acronyms too. We added a few, and I've seen users wanting to use more. There'll always be missing acronyms in this list for someone.

pallid bison
#

To be clear, I don't think there's an explicit convention to use or not use acronyms in JavaScript/GraphQL. Maybe there is, but I see both in the wild (in JS at least). So, if there's no convention not to use it in GraphQL, I'm not opposed to use it in our API, especially if it makes it easier given our Go codebase. However, I very much don't like how ambivalent the acronyms are, unless you have that list everywhere. Consider how a language like Python would know where to break the words, when you could have two one-letter words before another word. It's much clearer when you only use an uppercase when it's the start of a new word.

#

You'd be able to try it, with this fix:

dagger call -m github.com/helderco/daggerverse/codegen introspect get-type --name=Container fields name-words

But the idea there was for codegen to return my JSON response, so python wouldn't care about the casing, but those that do would still be able to preserve the casing if needed. For example: My + JSON + Response = MyJSONResponse; or My + Json + Response = MyJsonResponse. Since the words are already separate, there's no ambivalence.

pallid bison
#

My function turns that into this:

Screenshot_2024-08-09_at_22.16.41.png
pallid bison
novel moon
#

^still curious about the flakiness in the test
I probably had wrong choice of words there in saying "race condition". What I meant instead is depending on where the strcase package is defined and imported (before and after the change), that init function is getting called and changes the behavior of how JSON is handled.

#

I guess the question is what configuring an acronym does: does it merely say "preserve JSON when you see it", or does it also say "convert Json to JSON"?

I guess this is a behavior difference in the new lib (notice the 3rd example)

(before acronym, existing) strcase.ToPascal(Json) ->  Json
(before acronym, new lib ) strcase2.ToPascal(Json) ->  Json

(before acronym, existing) strcase.ToPascal(JSON) ->  Json
(before acronym, new lib ) strcase2.ToPascal(JSON) ->  Json

(after acronym, existing) strcase.ToPascal(Json) ->  Json
(after acronym, new lib ) strcase2.ToPascal(Json) ->  JSON

(after acronym, existing) strcase.ToPascal(JSON) ->  JSON
(after acronym, new lib ) strcase2.ToPascal(JSON) ->  JSON