getgauge / gauge-dotnet

C# runner for gauge + DotNet standard 6.0-8.0
Apache License 2.0
27 stars 21 forks source link

Add Concept related notification to a code Runner #203

Closed PiotrNestor closed 1 month ago

PiotrNestor commented 9 months ago

When using a lot of concepts it's useful to get an indication when a concepts starts / ends in a callback hook. These notifications can be then used to create a readable and helpful execution trace that shows concept info and not just expanded steps info.

There is a possible implementation:

  1. Gauge proto update to add Concept Start / End to Runner: concept-start-end-notify-runner
  2. Gauge update to send Concept start / end to Runner: concept-start-end-notify-runner
  3. dotnet update to call step hooks for the above: concept-start-end-notify-runner

@sriv What is the preferable mechanism to indicate to the step hook that this is a concept step?

jensakejohansson commented 9 months ago

@sriv To elaborate a bit on @PiotrNestor message:

We send message now also to runner for concept start / ending. We want to know in our test code when a new concept start (at the moment for logging purposes). Then questions we ask our selves now is which (if any) of the 2 options we should go for:

  1. Expand the context/step information so in the hooks Before/AfterStep we can check if the step is actually a concept.
  2. Introduce a new hook for Before/AfterConcept.

It seems easier to go for option 1 above, but we want your input. Thanks!

sriv commented 9 months ago

Runners do not understand Concept as it stands today. Concept steps are resolved and inlined by gauge and an execution sequence is determined (i.e. hooks and steps). Runner is told to execute the code in the corresponding sequence.

So basically, by the time the runner springs to action, the concepts are replaced by the contextual steps

So I believe getting runners to be aware of concepts isn't necessarily the best thing to do. Also worth noting that a suite can have nested concepts, i.e. a concept invoking another concept.

So I am more inclined towards this

Expand the context/step information so in the hooks Before/AfterStep we can check if the step is actually a concept

This would be simpler to implement as well IMO.

I'd be interested in your ideas on how to enrich the context to include this information.

jensakejohansson commented 9 months ago

👍 Then we might try to propose a solution for that option. We'd generally like to enrich the context with as much information as possible from Gauge and we can start with this.

We also have code for enriching context with information about retry information (current try / max retries). This is useful for us since it affects how we handle the state of our system under test (if it's a retry we might need to reset DB information to be able to have a successful retry, but we don't want to do it on the first execution of a test due to performance implications) - but that's an other PR.

PiotrNestor commented 9 months ago

@sriv I've updated the relevant gauge-proto gauge gauge-dotnet gauge-csharp-lib

  1. gauge sends if isConcept == true for a concept step, and false otherwise
  2. gauge-csharp-lib is updated with additional data in 'StepDetails'
  3. gauge-dotnet tries to create the StepInfo with the additional data. This fails and it's unclear to me what is the problem
        Error Message: Constructor on type 'Gauge.CSharp.Lib.ExecutionContext+StepDetails' not found.
        Stacktrace: 
           at System.RuntimeType.CreateInstanceImpl(BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture)
           at System.Activator.CreateInstance(Type type, Object[] args)
           at Gauge.Dotnet.Wrappers.ActivatorWrapper.CreateInstance(Type t, Object[] args) in G:\work\Programs\syve-gauge\gauge-dotnet\src\Wrappers\ActivatorWrapper.cs:line 16
           at Gauge.Dotnet.ExecutionInfoMapper.StepFrom(StepInfo currentStep) in G:\work\Programs\syve-gauge\gauge-dotnet\src\ExecutionInfoMapper.cs:line 60
           at Gauge.Dotnet.ExecutionInfoMapper.ExecutionContextFrom(ExecutionInfo currentExecutionInfo) in G:\work\Programs\syve-gauge\gauge-dotnet\src\ExecutionInfoMapper.cs:line 31
           at Gauge.Dotnet.HookExecutor.Execute(String hookType, IHooksStrategy strategy, IList`1 applicableTags, ExecutionInfo info) in G:\work\Programs\syve-gauge\gauge-dotnet\src\HookExecutor.cs:line 47
        Error Message: Constructor on type 'Gauge.CSharp.Lib.ExecutionContext+StepDetails' not found.

But the dll has the relevant constructor as far as I can see

image

sriv commented 9 months ago

The Gauge.Csharp.Lib dll exists in 2 places :

  1. With the runner (i.e. in the $GAUGE_HOME/plugins/dotnet/<version>/bin/net8.0
  2. In the test project as a nuget dependency

You will need to update it at both the places. I suspect that the test project is referring to an older version of Gauge.CSharp.Lib and hence is not able to find the type

PiotrNestor commented 9 months ago

@sriv Thanks! I've cleared all old Gauge.Csharp.Lib dlls and rebuild, re-installed the dotnet runner Now it works :)

PiotrNestor commented 9 months ago

@sriv We're close to a working update. The important question now is that we have moved 'Gauge.CSharp.Core' library from the 'gauge-csharp-core' project / repository to the 'gauge-dotnet' src project.

The reasons are:

Is this significant change OK with you?

sriv commented 9 months ago

Thank you for the update @PiotrNestor .

I agree with all the reasons you've listed for merging Core. The main driver was not different runner but Gauge-VisualStudio plugin (which is now deprecated in favour of VSCode). Gauge-visualstudio and the dotnet(earlier gauge-csharp) runner both used Core.

That said, I am wondering now if Core belongs in gauge-dotnet or should be move it to gauge-proto? go and java have their generated code residing in/triggered via gauge-proto, so I wonder if we should be consistent that way?

The benefit I see is that gauge-dotnet can be free of any proto dependency, no need for protoc and the git submodules.

PiotrNestor commented 9 months ago

@sriv @jensakejohansson

Yes, I see your point. There are two main options then:

  1. Gauge Csharp Core as a nuget in 'gauge-proto'

    • (plus) generated from 'proto' files that reside in the same Git repo.
    • (plus) simple script using protoc that generates csharp files from proto definitions
    • (plus) no need for Git submodule in 'gauge-dotnet'
    • (minus) Gauge Csharp Core nuget needs to be built, maintained, distributed to the Nuget infrastructure
    • (minus) somewhat more complex script that handles the nuget
  2. Gauge Csharp Core as part of 'gauge-dotnet'

    • (plus) no Gauge Csharp Core nuget is needed
    • (plus) Simple script using protoc that generates csharp files from proto definitions
    • (minus) generated from 'proto' files that reside in another Git repo.
    • (minus) Git submodule reference to 'gauge-proto' is needed

'gauge-dotnet' code depends on 'gauge-proto' generated csharp anyways 'gauge-java' is actually using git submodule to 'gauge-proto'. Java from proto is handled in 'gauge-java'

Presently I have used option 2. in the latest update so I'm voting for that :)

sriv commented 9 months ago

Thanks for the summary. Let's go with option 2 since you already have the changes.

We can always revisit this if there is another consumer for Core in the future.

Thanks again for the good work!

PiotrNestor commented 9 months ago

@sriv

I'm reviewing the required scripts for generating the csharp files from gauge-proto I'm not very familiar with the 'paket' tool and it's unclear what 'dotnet paket' command is used to get 'Grpc.Tools'

So far:

  1. I've updated ./config/dotnet-tools.json
    {
    "version": 1,
    "isRoot": true,
    "tools": {
    "paket": {
      "version": "8.0.3",
      "commands": [
        "paket"
      ]
    }
    }
    }

    paket.dependencies

    
    source https://www.nuget.org/api/v2

nuget Google.Protobuf nuget Grpc.AspNetCore.Server


2. dotnet tool install paket
3. dotnet tool restore
Tool 'paket' (version '8.0.3') was restored. Available commands: paket
4. dotnet paket restore
5. gen-proto.ps1 
- needs packages\build\Grpc.Tools\tools\windows_x64\...
It's unclear how to get 'Grpc.Tools'

I can get manulally: grpc.tools.2.60.0.zip
and create 'packages\build\Grpc.Tools'

Then it's possible to generate 'src\Gauge.CSharp.Core
sriv commented 9 months ago

We don't need to introduce paket into gauge-dotnet. The Core project was developed before .net core and some of the things you see may be because of legacy reason.

My suggestion would be that we generate the classes using the same mechanism but lets try installing Grpc.Tools via the regular dotnet add package Grpc.Tools command

PiotrNestor commented 9 months ago

@sriv @jensakejohansson

  1. OK, added grpc.tools dependency dotnet add package Grpc.Tools --version 2.61.0
  2. I'm still using then (updated): gen-proto.ps1, genproto.sh to create the .cs from .proto
  3. Possibly .csproj could have the proper configuration to create the .cs from .proto but for now now I cannot get it to work

The remaining question is: What's the 'gauge-dotnet' master status? The 'async' support needs to be fixed or removed there in order to get this PR to work

sriv commented 9 months ago

My theory is that gauge-dotnet works fine with async, the problem is in gauge-csharp-lib and the way it handles datastores. I am trying to verify this. I will try to get back with either a fix or revert the changes in master so that we can be unblocked for this.

sriv commented 9 months ago

It looks like I may not have the time to investigate the async support failure fully (although I still am looking at it in intervals).

Meanwhile, I have reverted the commit for async in master - please, can you rebase and continue this work? It should unblock us from making a release. Thanks

jensakejohansson commented 9 months ago

Thanks, we were just about to ask about your plans. Then we'll provide a PR soon where we have enriched the context a bit.

  1. We send step information also for "Concept-steps". A field in context (bool) indicated if its a concept-step or not.
  2. Also added fields for error message and strack trace. So they are available to context within the running test if failure occurs, in our case very useful for logging purposes.
chadlwilson commented 1 month ago

Believe this was addressed in various PRs subsequently merged.