[Perf framework] No-op methods for setup and cleanup and the global ones

[HarshaNalluru]

• Defines globalSetup, globalCleanup, setup and cleanup
• I don't see a reason for them to be optional, so changing that.
• Making run and runAsync abstract

To address @mikeharder's comment at #12737 (comment)

[HarshaNalluru]

Question: Should we remove this as we only ever offer async methods for our APIs?

[HarshaNalluru]

In a future PR, as it would involve modifying the framework.

[mikeharder]

Yes, I would remove all the sync code in a separate PR.

[sadasant]

A no-op implementation of the base method seems misleading to me. These methods do nothing by default nor need to be implemented for the PerfStress framework to work. If we wanted to do stuff at that level in the core, we could do it outside of a base definition of these methods.

Can we not do this?

@mikeharder @HarshaNalluru

[mikeharder]

@sadasant: I consider this a question about the recommended pattern of virtual methods in TS, not specific to the perf framework. In .NET, there are two ways to allow subclasses to override behavior in the base class:

1. A virtual method which requires some implementation (though it can be a no-op). The contract is that derived classes must call the base method, even if it's currently implemented as a no-op, since it may contain code in the future.
2. An abstract method which cannot have any implementation, and a subclass must override the method in order to compile.

Does TS commonly use a different pattern for virtual methods?

[sadasant]

@mikeharder just for the record, we talked and I understand where you come from and I'm not against this approach. I'm deferring this decision to @HarshaNalluru and @xirzec 👍 I know what they decide will be ok.

[xirzec]

My take on this: TS doesn't have a virtual keyword, so we have no way for the compiler to force folks to call super when they override a method on the base class. Since we can't enforce it, we have to assume that someone might forget to call it.

As we probably don't want the perf stress framework to fail horribly if someone forgets to call super.globalSetup(), we already have to specially handle any logic that needs to execute before their globalSetup is called without relying on them calling into the base class.

So there's no real compelling reason for me to force users to write super.globalSetup() at all. By not requiring it, we save them a line of code and the mental overhead of remembering to write it.

[mikeharder]

@xirzec: I'm not sure how things usually work in the JS/TS ecosystem. But in .NET, if you override a virtual method, you are responsible for understanding the contract and whether or not you need to call the base method. For example, if you override a method like Dispose(), you better call base.Dispose() otherwise your base class will not be properly disposed and could leak memory, resources, etc. However, if you override a method like ToString(), you probably don't care how your base class implemented it or want to include their string in your output, so you don't need to call base.ToString().

In .NET, I would say you should default to calling base unless you know it's safe not to. If a method returns void (or Task) you should almost always call base, since it's likely a method that "does something" instead of "returns a computation".

I would expect inheritance in JS/TS to have the same expectations, but if not then we might need to take a deeper look at the whole Perf framework, since it was designed around .NET-style inheritance.

[mikeharder]

@xirzec: Note this is about more than just the PerfStressTest base class, it's about the full hierarchy of classes extending it. For a specific example, StorageBlobTest implements globalSetup() by creating a storage container, and then StorageBlobDownloadTest extends this by calling super.globalSetup() and then additionally uploading a blob. If StorageBlobDownloadTest does not call super.globalSetup(), the test will not run.

https://github.com/Azure/azure-sdk-for-js/pull/12737/files#diff-f4f4dce4fcdcf96c7feb9eba5fdd92a752984e0a4b2a788d69babeae7547d2dcR34

[HarshaNalluru]

@mikeharder

Classes are a relatively newer concept to JS. As they are not widely used as much as in .NET at least, calling base class' methods such as super.globalSetup() might feel like an overhead for the JS users while it is more natural for .NET.

That said, I feel the model of the framework is perfectly fine. If the developer who'd write the test with a base class such as StorageBlobTest and a child class StorageBlobDownloadTest, it is up to them on how they want to deal with the methods that are defined in such a base class and calling them from a child class, as they would be more conscious of how the base class and the methods are defined.

When it comes to the PerfStressTest class in the framework, it might be simpler for the users if they are not forced to specify calls such as super.globalSetup() in their tests as they'd have to keep the perf framework in mind.
In a call with @xirzec, he suggested an idea where we could make all the methods in the PerfStressTest as abstract and let users implement their own globalSetup, cleanup, etc with their base classes and child classes. This would mean that any default code if added in the perf framework will not be part of the PerfStressTest class but would be present in PerfStressProgram.run().

While I totally love the concept of "understanding the contract" and "calling the base method" from the .NET perspective, and though it is completely valid to follow the same pattern in TS, it might be a foreign concept relatively as pointed out.

---

• The way I see, we can tweak the current framework in 3 ways, here are the options being considered
  • Link to the GIST - The gist has the perf-stress base class for the three options along with the examples of how it would be used along with the storage base class and storage-download test.

[mikeharder]

@HarshaNalluru: Of the 3 options in the Gist, my preference is 3 (require all subclasses to call super), followed by 1, and then 2. I don't like forcing all perf tests to implement all 4 setup/cleanup methods since they are often empty. But whatever the JS team decides is fine with me.

In example 3, I think there is a bug where StorageBlobTest is not calling super.

[HarshaNalluru]

Thanks, @mikeharder!
Updated the option 3 in Gist with super.globalSetup() call. (On a funny note, this is exactly what @xirzec / @sadasant are worried about, missing the super.globalSetup() call)

I'll check with @xirzec and finalize.

[xirzec]

require all subclasses to call super

I'm curious how you plan to accomplish this. If you can't make it a TS error, I submit you can't "require" it in any meaningful way.

[HarshaNalluru]

Yes, we won't be able to force the users into calling methods such as super.globalSetup of the perf framework.

So, to conclude, going with Option-1 seems to be the best as it doesn't add the overhead when compared to Option-3 and it doesn't force users to specify empty methods like Option-2.

And since Option-1 is what we currently have in the framework, I'm closing this PR.
For the other issue that is discussed in this PR related to removing the "run" method - opened a new issue #13209.