Skip to content

Handle case when underlying step returns Task<T> masked as non-generic Task#343

Merged
neparij merged 1 commit into
allure-framework:nunit-refactorings-1from
eranikid:nunit-refactorings-1
May 3, 2023
Merged

Handle case when underlying step returns Task<T> masked as non-generic Task#343
neparij merged 1 commit into
allure-framework:nunit-refactorings-1from
eranikid:nunit-refactorings-1

Conversation

@eranikid

@eranikid eranikid commented Apr 6, 2023

Copy link
Copy Markdown
Contributor

Context

Current code for NUnit.Allure.Core.Steps.AllureStepAspect in nunit-refactorings-1 branch contains a flaw:

if (_typeTask.IsAssignableFrom(returnType))
{
var syncResultType = returnType.IsConstructedGenericType
? returnType.GenericTypeArguments[0]
: _typeVoidTaskResult;
return AsyncHandler.MakeGenericMethod(syncResultType)
.Invoke(this, new object[] { target, args, metadata, stepName, stepParameters });
}

tl;dr - It always casts return value of methods with Task declared return type to Task<VoidTaskResult>, which is not always valid

Here, if the step is declared as returning Task, we're making a generic method using type VoidTaskResult as a value for type argument T.
This later leads to the return value of the method being cast to Task<VoidTaskResult>

var result = await ((Task<T>)target(args)).ConfigureAwait(false);

The problem is - method returning Task can actually return, for instance, Task<string>, as Task<T> is subtype of Task. This makes the following C# code valid:

public Task MyOuterFn() => MyInnerFn();

public Task<string> MyInnerFn() => Task.FromResult("hello");

But attempting to cast Task<string> to Task<VoidTaskResult> throws InvalidCastException.

To address this issue, I propose to have a separate handler for Task-returning operations, and a separate handler for Task<T> returning operations

Suggestions are welcome on different designs, or on unit-tests I could possibly write for this

Checklist

@eranikid eranikid marked this pull request as ready for review April 6, 2023 12:33
@neparij

neparij commented May 3, 2023

Copy link
Copy Markdown
Contributor

Merging this PR. Will be moved to Allure.Net.Commons with some refactoring.

@neparij neparij merged commit 686605d into allure-framework:nunit-refactorings-1 May 3, 2023
@delatrie delatrie added the type:bug Something isn't working label Sep 25, 2023
neparij added a commit that referenced this pull request Oct 16, 2023
* Adds AllureBefore and AllureAfter to Allure.NUnit package

* NUnit: Ability to change testresult on fixture executions

* Preserve testResult.start time if it exists in update container

* SpecFlowNunit: add custom labels

* Fix missing output attachments in NUnit adapter

* Added step logging for NUnit (closes #312)

* Fixed NullPointerException if NUnit StepLogger was null (#315)

* embed untracked sources

* build process update

* NUnit Fix async behaviour in [AllureStep] aspect + Allow '{obj.prop}' placeholders in [AllureStep] step name (#329)

Co-authored-by: Кабанов Константин Юрьевич <kykabano@mts.ru>

* Package assets fix

* NUnit - Add an ability to save OneTimeSetUp step results (#333)

* Handle case when underlying step returns Task<T> masked as non-generic Task (#343)

* Unify xunit and nunit step acpects

* Replace allure-xunit's custom attributes with runner reporter (fixes #344) (#366)

* Add support for native xunit attributes

* Fix AllureXunitTheoryAttribute base class

* Fix missing theories

* Fix missing parameters and fixture duplication for some tests

* Add support for static test functions

* Update xunit examples. Obsolete allure-xunit attributes

* Update message sink to match new steps implementation

* Remove irrelevant log method from runner

* Bump harmony to 2.3-prerelease.2. Warn if no args reported

Change Lib.Harmony dependency to 2.3-prerelease.2 for .net 7 support.
Add warning if the patch via harmony didn't work for theory args and
args aren't reported on the testcase level.

* Fix reference to lib.harmony to be listed by dotnet

* Factor out allure-related code. Tidying up obsolescence

  - Allure-xunit's code that creates, updates, starts, stops and writes
    allure objects is factored out from AllureMessageSink to
    AllureXunitHelper .
  - Removal of AllureXunitHelper's public methods was reverted. The
    methods are marked as obsolete.
  - Add missing await to an allure-xunit example to suppress the
    compilation warning.
  - Mark obsoleted allure-xunit attributes as non-browsable to prevent
    them from appearing in IDE's suggestions.

* Add deprecation info and known issues to allure-xunit README

* Fix issue links in allure-xunit readme

* Fix obsolescence message for AllureXunitHelper's public methods

* New context-based state management model for allure-csharp (#371)

* Add AllureContext lass to isolate allure state from threads and async tasks

* Add concurrency tests. Reformulate context in terms of active/inactive

* Add tests on the context capturing by child threads/tasks

* Modify lifecycle/storage to use new context model. Add context get/set API

* Make CoreStepsHelper use AllureLifecycle's context

* Make lifecycle thread safe. Fix multithreading issues in lifecycle tests

* Several improvements on context. Allure-xunit migration

- Add bool props to test context to context's public API
- Fix context string conversion
- Migrate allure-xunit's code to the new context implementation

* Bump C# language version to 11 for all projects

* Enable null checks in some classes of allure-xunit

* Make lifecycle methods with explicit uuids obsoleted

* Minor changes in context and lifecycle

  - Improve error messages and doc comments
  - Add DebuggerDisplay for context
  - Uuid in string representation of context if names are empty
  - Old storage uuid-based methods obsoleted
  - Make RunInContext return modified context
  - Replace unsupported container nesting with test nested
    into each container in chain
  - Container and test starting now add objects into storage by
    uuids (transition period)

* Rewrite Allure.NUnit to support the new context scheme

* Fix Allure.XUnit usage of RunInContext

* Rewrite Allure.SpecFlowPlugin to support new context scheme

Additionally:
  - Change broken status to failed for some scenario and step conditions
  - Add extra test case instead of changing state of existing one in case
    after feature failed
  - Enable nullable check project-wide

* Separate context from storage

  - AllureContext moved to Allure.Net.Commons namespace
  - Context and storage are separated; Storage will be removed later
  - File scope namespaces for lifecycle and context

* Fix obsolete messages. Add file scoped namespaces

* Revert obsoleted StopFixtureSuppressTestCase to be intact

* Add new info to commons README

* Fix invalid test start. Remove rudimentary test start check

* Remove dotnet SDK 3.1 installation from build pipeline

* Change .NET SDK version to 7.0 in build pipeline

* Github pipelines fix

  - bump actions/checkout to 3.5.3
  - bump actions/setup-dotnet to 3.2.0
  - add dotnet 3.1 and 6.0 SDKs back to build and test
  - Change --no-restore to --no-build in dotnet test step

* Rename a placeholder scenario

* Implement requested changes for PR #371

  - Add allure directory clean before all tests in Allure.NUnit.Examples
  - NUnit.Allure.Core.AllureExtensions.AddScreenDiff is back
    and marked as obsolete (it just directly calls
    AllureLifecycle.AddScreenDiff now).
  - Intendation in Allure.SpecFlowPlugin.csproj fixed.

* NUnit: Fix inconsistent one-time fixtures behavior (fixes #286, #374 and #375) (#380)

* Fix inconsistent one-time fixtures behavior (fixes #374)

* Fix crash when running test from empty namespace (fixes #375)

* Fix indentation for AllureAsyncOneTimeSetUoTests.cs

* Allure-xunit: support for the second reporter and xunit 2.5.0. Updated packaging and AspectInjector (fixes 368) (#382)

* xunit: change sink base, remove redundant err logging

* Add ability to compose xunit runner reporters (implements #368)

* Extract allure-xunit runner reporter into separate project

* Factor out common props. Bump and tune injector

* Reflect new project structure in solution

* Enable fully deterministic build in CI

* Add startup message to Allure.XUnit

* Fix second reporter property name in config for Allure.XUnit's example

* Rewrite how to run section and fix allure-xunit's README

* Fix comment in Directory.Build.props

* Revert AspectInjector build optimization

* Add link to allureConfig.json issue

* Fix release workflow to apply version via .props

* Change net.commons description

It's quite different from allure-java-commons now.

* Improve readability of ResolveExplicitReporter

* Fix typo in allure-xunit README

* release 2.10.0-preview.1

* set next development version 2.11

* Selective run support: commons, nunit, xunit (#390)

* Implement selective run commons

* Change GetAllureId to take an enumeration

* Implement selective run for allure-nunit

* Add a shorthand for testplan's IsMatch

* Implement selective run for allure-xunit

* Move TestPlan property to commons. Rename selection check method

* Allure-specflow: selective run support (#392)

* Fix regression: non-working lambda-steps and lambda-fixtures API (#393)

* allure-xunit: provide default config when allureConfig.json is missing

Fixes #381

* Fix async step/fixture wrapper functions

  - make async Step, Before and After obey .NET's ExecutionContext capturing rules
  - refactor CoreStepsHelper to be more testable
  - write tests for CoreStepsHelper's Step, Before and After

More tests are needed to fully cover CoreStepsHelper.

Fixes #383
Fixed #388

* Add async suffix to private step methods

This helps to make distinction more clear to a reader.

* Fix identification properties and parameter formatting (#395)

* Bump NUnit for test and example projects

* Add json schemas for allureConfig and testplan

* Fix schema for allure-xunit and allure-specflow config

* Add legacy naming switch to config

* Update schema links to current branch

* Add schema links update task to release workflow

* Implement UUID and method-based fullName algorithms

* Common fns to calculate testCaseId, historyId and parameter value

* Add docstrings to new id-related functions

* Add function to create fullName from class

* Fix allureConfig item type for allure-nunit examples

* Add missing setter for UseLegacyIds config property

* Add framework label factory

* Fix uuid, fullName, historyId, testCaseId for allure-nunit

UseLegacyIds option is honored.

Additionally:
  - fix uuid for containers
  - fix missing historyId for ignored tests. Same id rules apply as for non-ignored tests (fixes #345)
  - add NUnit 3 framework label to test results
  - remove suites from AllureDisplayIgnored (now suites can be applied directly)
  - fix parameter names for parametrized test methods

* Apply custom formatters to step title and parameters

Now step title interpolation and step parameter value conversion uses the same
algorithm as test parameter conversions.

Fixes #377

* Add language label to commons and NUnit

* Fix uuid, fullName, historyId, testCaseId for allure-xunit

Legacy identifiers switch is honored.

Additionally:
  - Add framework and language labels to allure-xunit test results
  - Fix test parameters formatting. Custom type formatters are also used now

* Add no-formatters step helper overloads back to public API

* Revert autoedit this removal from StepFunctionTests.cs

* Fix uuid, fullName, historyId, testCaseId for allure-specflow

Legacy ids switch is honored.

Implements #387

* Implement requested changes

* Lower the required AspectInjector version for users back to 2.8.1 (workaround for #391) (#396)

* Add workaround for #391

* Comment AspectInjector workaround step in publish.yml

* Add Rosetta warning in package READMEs. Fix allure-nunit readme

* Update allure-xunit and allure-specflow README (#397)

---------

Co-authored-by: qameta-ci <qameta-ci@qameta.io>
Co-authored-by: Andrey Gurenkov <undron@users.noreply.github.com>
Co-authored-by: Constantine <overlord1983@mail.ru>
Co-authored-by: Кабанов Константин Юрьевич <kykabano@mts.ru>
Co-authored-by: Alexander Kulakov <a.kulakov@dodopizza.com>
Co-authored-by: Dmitry Pavlushin <d.pavlushin@dodopizza.com>
Co-authored-by: Maxim <17935127+delatrie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

theme:nunit type:bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants