Skip to content

WIP: Initial warning about cross-process marshalling: Issue #996 - #995

Closed
craigfowler wants to merge 1 commit into
nunit:masterfrom
craigfowler:patch-1
Closed

craigfowler wants to merge 1 commit into
nunit:masterfrom
craigfowler:patch-1

Conversation

@craigfowler

Copy link
Copy Markdown

This catches me out every darn time! I feel like it should be more obvious and pointed out. In this case I've added an object as a parameter which is attached to an event bus which is maintained outside of the test execution. This means that the event bus has an affinity with the "test discovery & creation" process wheras the things submitting events to it are in the "test execution" process. Of course, this results in two 'copies' of the event bus, and any listeners to it (which were attached from the "discovery & creation" process) don't actually receive any of the events sent by the test execution.

I'm not 100% sure this is factually correct yet; I could use some assistance from maintainers to be confident that it's right. At the moment this PR is more of a reminder to myself to look further into it and improve upon it.

This catches me out every darn time!  I feel like it should be more obvious and pointed out.
@craigfowler craigfowler changed the title Initial warning about cross-process marshalling WIP: Initial warning about cross-process marshalling: Issue #996 Dec 16, 2024
@craigfowler

Copy link
Copy Markdown
Author

I've opened #996 to describe the initial problem I'm trying to solve. There are probably other places where this could be signposted-from, such as the docs for those interfaces/methods which are executed "in test suite/case discovery/creation"-land.

I could use some help from maintainers to point that out, as well as to fact-check that I'm even correct (based on a half-recalled explanation from long ago).

@OsirisTerje

OsirisTerje commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

@stevenaw @manfred-brands

This doesn't seems right. @CharliePoole said on #996 that parameter values from an ITestBuilder are not marshalled to another process..

What could be written (according to Charlie) paraphrased : explain at user level "that builders run at load time, that the test is fixed before execution starts, and that runners may load the assembly more than once, so builder code can run more than once and must not depend on state outside the test. The VS discovery/execution split belongs in the adapter docs rather than on the Custom Attributes page."

@OsirisTerje OsirisTerje closed this Oct 9, 2026
@CharliePoole

Copy link
Copy Markdown
Member

The distinction between discovery and execution "phases" predates a lot of the implementation details that have been mentioned. For example, it is true that certain versions of NUnit have done everything in one process and other versions have used different processes. However, in all versions I've been involved with (2.0 through present day) the definition of the tests, including all their arguments, is completed and unchangeable before execution begins.

So...

"Discovery" (a term not used in early days, but what we now call it)

  • Recognizes and interprets all NUnit attributes
  • Finds all the classes containing test methods
  • Finds all the methods representing tests
  • Finds or generates all the data to be used as arguments for the tests
  • Creates the internal representation of all the tests in hieararchical form.

While "Execution"

  • Runs all one-time setups, setups, test bodies and teardowns
  • Produces results and reports

I've always found it useful to imagine that "Execution" takes place about a century after "Discovery." :-)

Long ago, we had an idea for "dynamic" tests. Our specific use of "Dynamic" was intended to mean something like "discovered during the execution phase." I think the reason we never implemented it was because it would mean re-architecting the entire discovery/execution pipeline, a monumental change.

Another note... Visual Studio also makes a distinction between discovery and execution. Their distinction is not the same as NUnit's, which is one reason why our VS adapter is an "adapter."

@manfred-brands

Copy link
Copy Markdown
Member

@OsirisTerje Did you closed this accidentally?

There is last paragraph in the TestCaseSource documentation

By design, no communication is possible between the load and execution phases except through the tests that are created.

What I couldn't find is that the values returned for testcases should be repeatable.

  • Don't use Random inside a TestCaseSource member.
  • Don't use side-effects, like using static members, as the test case source is evaluated for every method that has the attribute.

@CharliePoole

Copy link
Copy Markdown
Member

@manfred-brands The issue is hard to deal with in the documentation for a single attribute because it cuts across a number of attributes. The same considerations apply to FixtureSource and ValueSource. In fact, they apply to all attributes that generate data, e.g. TestCase, except that the compiler prevents you from using anything but const values in most other cases. I suppose there could be a few paragraphs about data generation constraints, which we would insert in each relevant attribute description.

The error I've seen frequently statics is to attempt to change the values in SetUp. Of course, by the time SetUp runs, the values have already been used and changing them has no effect.

@OsirisTerje

OsirisTerje commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

@manfred-brands

Did you closed this accidentally?

No, I closed it because the PR change is not correct.

I understand we need to fix the wording around multiple other attributes, but it seems to be that it should be another PR resolving #996, but also that the title there should be ... refined.

I'm trying to summarize what you two have posted above in #996 and change the title there.

UPDATE:
It seems better to raise a new issue and close #996 , otherwise we take over what is written by the reporter there. I'll raise that issue and point back to the reporters.

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.

4 participants