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

Add testing platform architecture #41735

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

@MarcoRossignoli
Copy link
Member

I don't see the image and the mermaid rendering correctly is it related only to the "preview"?

@Evangelink
Copy link
Member Author

@IEvangelist Are our docs supporting mermaid?

Copy link
Member

@IEvangelist IEvangelist left a comment

Choose a reason for hiding this comment

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

I'm still not done with this review, it's taking a really long time - been working on it for a while now. I'm going to comment on it, and return when I can focus a bit more. But for now, here's what I have.

Copy link
Member

Choose a reason for hiding this comment

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

I don't like this diagram. What's it trying to convey exactly? We need all the text in the image to be the same size, and the colors (typically pastel colors are better) aren't very appealing. While some of this is opinion, let's see if we can't do a bit better. I strive to use Excalidraw for consistency. You can then export the file for others to collaborate on, and add that to source control too. The legend should be on the bottom left, and instead of the shape + color, just used a square with the color. Does the shared bus also ever report back to the test framework?

Copy link
Member Author

Choose a reason for hiding this comment

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

Any extension to the new platform (including Test Framework) can publish or subscribe to data from the message bus.

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'll suggest a different design based on the tool you suggested, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

@Evangelink
Copy link
Member Author

I'm still not done with this review, it's taking a really long time - been working on it for a while now.

Thank you so much for everything you have reviewed already! We know this is a super big change, I have read it a few times and I am still not 100% happy with the results of what we wrote nor the structure but it's not always easy to get the abstraction of knowing all the implementation details.

Copy link
Member

@IEvangelist IEvangelist left a comment

Choose a reason for hiding this comment

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

There are still a few things that we'll need to update, this is starting to shape up though. Let's consider this 2/3 in terms of reviews. 🤓

@IEvangelist
Copy link
Member

I've checked back on this one a few times, there's still a lot of suggestions that haven't been resolved. I'll come back once those are addressed. /cc @Evangelink

@Evangelink
Copy link
Member Author

@IEvangelist Yes sorry! I have been pretty busy with other priorities, I'll get back here early next week!

@IEvangelist
Copy link
Member

@IEvangelist Yes sorry! I have been pretty busy with other priorities, I'll get back here early next week!

No worries, I completely understand. I just wanted to make sure I wasn't missing anything. I'm off all next week, so I'll check back the week of 8/12. Thank you, my friend! @Evangelink

@Evangelink
Copy link
Member Author

Hey @IEvangelist, could we resume work on this PR? We have finished integration with xUnit v3, NUnit, and we are getting close to finishing with F# Expecto.

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

Successfully merging this pull request may close these issues.

5 participants