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

Map incoming and outgoing requests #3314

Merged
merged 15 commits into from
Feb 22, 2022
Merged

Conversation

nohwnd
Copy link
Member

@nohwnd nohwnd commented Feb 1, 2022

Description

Pilot for UWP provider.

UWP provider deploys testhost to remote system, and runs tests there. We build into C:\projects\UwpTestProject\x64\AppX, and copy the contents of the folder to a remote system, under D:\ProgramData\UwpTestProject_x64_Debug_a90s87df234\

We then get a request that says to testhost: Run tests in C:\projects\UwpTestProject\x64\AppX\UwpTestProject.appxrecipe. That path does not exist on the remote system and the run fails.

So we map the path on incoming request to the correct path, and then re-map the paths back when we send the data back so VS can pick them up and match them up to their entries in test store database.

This PR adds --local-path <string> and --remote-path <string> to testhost. In the example above we would call testhost.exe --local-path C:\projects\UwpTestProject\x64\AppX --remote-path D:\ProgramData\UwpTestProject_x64_Debug_a90s87df234\.

Many of the updates rely on references being modified, and the code does not consider talking to data collector or other processes (they are not deployed with testhost anyway by UWP).

@MarcoRossignoli
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nohwnd nohwnd mentioned this pull request Feb 14, 2022
@nohwnd nohwnd marked this pull request as ready for review February 14, 2022 16:03

public IEnumerable<string> UpdatePaths(IEnumerable<string> enumerable, PathConversionDirection updateDirection)
{
var updatedPaths = enumerable.Select(i => UpdatePath(i, updateDirection)).ToList();
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to "execute" the update? I mean, is ToList() required? And if we want to actually do the change, is it worth to have the interface less generic? I don't like patterns where we put everything IEnumerable to then lways cast down to concrete list/array, that's just causing trouble.

Comment applies to most impl in this class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would love to do that this way, but the object contract is public, and the only thing I can do without changing it, is making this work in a terrible side-effectful way.

The select here will work just fine, ToList forces enumeration, but I changed it to use tolist and foreach in all places to make it more obvious that we do something that has side effects.

We don't cast down to anything here, I don't see where this would fail. Care to give an example?

Copy link
Member

Choose a reason for hiding this comment

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

When saying "casting down", I meant that I have often see codebases where you have something like:

var list = new List<int> { 1, 2, 3 };
var x = Update(list);
var y = AnotherUpdate(x);

private IEnumerable<int> Update(IEnumerable<int> en)
    => en.Select(x => x + 1).ToList();

private IEnumerable<int> AnotherUpdate(IEnumerable<int> en)
    => en.Select(x => x + 1).ToList();

So at the end we keep creating list while we already have one at first.

is making this work in a terrible side-effectful way.

Maybe an explicit foreach gives a better clarity on "side-effect"?

I would love to do that this way, but the object contract is public

Not sure to get it, you are adding the IPathConverter interface in this PR, so you could tweak it the way you want. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is "casting" up imho, IEnumerable is higher in the inheritance tree than List. But yeah, what you show is stupid design that is trying to be overly generic, and uses IEnumerable without using the lazy nature of it. I am not trying to do that here, I just use the types that are incoming in the request so I can go from var serialized = Serializer.Serialize(dto) to var serialized = Serializer.Serialize(_pathConverter.Update(dto)).

As for the side-effects, I am updating dtos that have a non-default constructor and non-settable properties, so I can't just create a new one with the updated data. I would love that, but instead I am forced to use side-effects to change the values in the incoming data (in most cases). Without that I would have to change the public api of those dtos, so they allow me to recreate them.

src/Microsoft.TestPlatform.CrossPlatEngine/Friends.cs Outdated Show resolved Hide resolved
…athConverter.cs

Co-authored-by: Amaury Levé <amaury.leve@gmail.com>
@nohwnd
Copy link
Member Author

nohwnd commented Feb 18, 2022

Forgot to initialize _pathConverter in both constructors which lead to discovery request deserialization that threw nullref exception, which obviously is just logged, and leads to hang...

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 we add some tests?

@@ -72,6 +78,9 @@ public TestRequestHandler() : this(JsonDataSerializer.Instance, new Communicatio
_onLaunchAdapterProcessWithDebuggerAttachedAckReceived = onLaunchAdapterProcessWithDebuggerAttachedAckReceived;
_onAttachDebuggerAckRecieved = onAttachDebuggerAckRecieved;
_jobQueue = jobQueue;

_fileHelper = new FileHelper();
_pathConverter = NullPathConverter.Instance;
}

protected TestRequestHandler(IDataSerializer dataSerializer, ICommunicationEndpointFactory communicationEndpointFactory)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe out of this PR, but could this ctor call the one above?

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked into that, but the other ctor does work that can be written in parameters, but it does not look nice. But we could probably refactor it to chain these ctors.

Comment on lines +54 to +55
string IDeploymentAwareTestRequestHandler.LocalPath { get; set; }
string IDeploymentAwareTestRequestHandler.RemotePath { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to have explicit interface implementation?

Copy link
Member Author

Choose a reason for hiding this comment

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

No strong reason, it just ensures that if we get rid of this interface in the future then all it's properties need to be removed from the type.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, only "downside" is that it forces you to make some cast in the code when you want to use it.

src/Microsoft.TestPlatform.CrossPlatEngine/Friends.cs Outdated Show resolved Hide resolved
Comment on lines +95 to +102
if (_requestHandler is IDeploymentAwareTestRequestHandler deployedHandler)
{
if (argsDictionary.ContainsKey(RemotePath) && argsDictionary.ContainsKey(LocalPath))
{
deployedHandler.LocalPath = argsDictionary[LocalPath];
deployedHandler.RemotePath = argsDictionary[RemotePath];
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Inner if could be merged with outer.

Suggested change
if (_requestHandler is IDeploymentAwareTestRequestHandler deployedHandler)
{
if (argsDictionary.ContainsKey(RemotePath) && argsDictionary.ContainsKey(LocalPath))
{
deployedHandler.LocalPath = argsDictionary[LocalPath];
deployedHandler.RemotePath = argsDictionary[RemotePath];
}
}
if (_requestHandler is IDeploymentAwareTestRequestHandler deployedHandler &&
argsDictionary.ContainsKey(RemotePath) &&
argsDictionary.ContainsKey(LocalPath))
{
deployedHandler.LocalPath = argsDictionary[LocalPath];
deployedHandler.RemotePath = argsDictionary[RemotePath];
}

Copy link
Member Author

Choose a reason for hiding this comment

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

It could, but for me the current one just reads better, you know that if the type is not IDeploymentAwareTestRequestHandler you can stop looking at it immediately.

Side note, do we have rule for placement of &&, to me it reads much easier if it is at the start of next line.

Copy link
Member

Choose a reason for hiding this comment

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

It could, but for me the current one just reads better,

Fine by me.

Side note, do we have rule for placement of &&, to me it reads much easier if it is at the start of next line.

We don't. I need to look if there is something in StyleCop for that. I am really happy to see you also prefer them at the beginning :)


public IEnumerable<string?> UpdatePaths(IEnumerable<string?> paths!!, PathConversionDirection updateDirection)
{
return paths.Select(i => UpdatePath(i, updateDirection)).ToList();
Copy link
Member

Choose a reason for hiding this comment

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

Just to match same style as other implementations:

Suggested change
return paths.Select(i => UpdatePath(i, updateDirection)).ToList();
paths.ToList().ForEach(i => UpdatePath(i, updateDirection));
return paths;

Copy link
Member Author

Choose a reason for hiding this comment

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

This would not work, strings are immutable, so you would create a new string and output it to null, so the original string would not be updated.

@nohwnd
Copy link
Member Author

nohwnd commented Feb 21, 2022

Could we add some tests?

Yes, but let me try it first together with winui in a real deployment, it is benign for all other cases that don't use the new functionality.

@nohwnd
Copy link
Member Author

nohwnd commented Feb 21, 2022

I am not even sure if this will be the final design, or if we can steer them towards using remote test environment, and roll this back altogether. But I need fix for the next preview.

Co-authored-by: Amaury Levé <amaury.leve@gmail.com>
@nohwnd nohwnd enabled auto-merge (squash) February 21, 2022 10:04
@nohwnd nohwnd disabled auto-merge February 21, 2022 10:04
@nohwnd nohwnd merged commit fda24d2 into microsoft:main Feb 22, 2022
@nohwnd nohwnd deleted the remote-testhost branch February 22, 2022 08:39
@nohwnd
Copy link
Member Author

nohwnd commented Feb 22, 2022

Merged this because it is benign and went through all reviews. I will need more changes to make the whole machinery work. 😥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants