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

Introduce RequestNotSupportedException #2704

Closed
wants to merge 2 commits into from
Closed

Conversation

MarcoRossignoli
Copy link
Contributor

@MarcoRossignoli MarcoRossignoli commented Apr 13, 2024

This exception should be used by the test framework author in case of unknown request.

@Evangelink feel free to refactor as you want. As soon as will be merged I'll it to the documentation.

This allow to evolve the testing platform with new request without side effect, for instance if we want to know the total amount of tests the will run introducing in future a CountTestsRequest to have a better interface(progress bar) we can add it and in case of RequestNotSupportedException exception we simply bail out that feature.

This doesn't mean that capabilities won't be used for this reason, but recognize if the adapter supports or not a request is useful to try to carry on the test session to the end "if possible".

@MarcoRossignoli MarcoRossignoli added the Area: Testing Platform Belongs to the Microsoft.Testing.Platform core library label Apr 13, 2024
@MarcoRossignoli
Copy link
Contributor Author

cc: @Evangelink

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Could it make sense to inherit from NotSupportException instead?


namespace Microsoft.Testing.Platform.Requests;

[Serializable]
Copy link
Member

Choose a reason for hiding this comment

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

I think we should avoid this one based on docs updates: dotnet/docs#34893

namespace Microsoft.Testing.Platform.Requests;

[Serializable]
public class RequestNotSupportedException : Exception
Copy link
Member

Choose a reason for hiding this comment

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

Shall we have the usual ctors? Message and inner exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering if make sense...the platform doesn't needs any message, I think we will take the exception and we will do something like

  • print in console
  • crash
  • move on if we're good with it...

So I wonder what the test framework author could put in that message.

But I won't oppose if you want it to "standardize" or have some inheritance in future, it's not sealed atm.

Copy link
Member

Choose a reason for hiding this comment

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

No you are right, we won't do anything about it.

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

I have been checking the code and I think we did a fail on our design and we should have had a return to the ExecuteRequest api https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform/TestFramework/ITestFramework.cs#L21

@MarcoRossignoli
Copy link
Contributor Author

I have been checking the code and I think we did a fail on our design and we should have had a return to the ExecuteRequest api https://github.com/microsoft/testfx/blob/main/src/Platform/Microsoft.Testing.Platform/TestFramework/ITestFramework.cs#L21

Discussed a bit off line, fine to me if we want to use NotSupportedException but I would not change the return value of the api, the only 2 scenario come to my mind to return prematurely from this method is for a unhandled exception or missing request support.

@MarcoRossignoli
Copy link
Contributor Author

Close as not needed atm, we can use NotSupportedException

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Testing Platform Belongs to the Microsoft.Testing.Platform core library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants