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

Allow a Turntable to load a Vinyl on demand #31

Closed
RuiAAPeres opened this issue Feb 19, 2016 · 19 comments
Closed

Allow a Turntable to load a Vinyl on demand #31

RuiAAPeres opened this issue Feb 19, 2016 · 19 comments

Comments

@RuiAAPeres
Copy link
Member

Looking at this usage from a usage POV, I seem myself creating the Turntable with a given configuration and then on each test case just load a particular vinyl and use it. As it is right now, you have to create a turntable every time.

@RuiAAPeres RuiAAPeres self-assigned this Feb 19, 2016
@RuiAAPeres
Copy link
Member Author

We are also considering setting the Configuration at global scale instead, so you don't have to pass one everytime you create one. cc @SandroMachado @zamith

@zamith
Copy link

zamith commented Feb 19, 2016

@RuiAAPeres I think I'm missing some context here. Can you explain in more detail what you plan to do? Maybe some code samples?

@RuiAAPeres
Copy link
Member Author

@zamith code examples can be found at the Intro

@zamith
Copy link

zamith commented Feb 19, 2016

@RuiAAPeres Ok, so let me see if I got this straight. You create a turntable that can have more than one vinyl and that vinyl can have multiple tracks? Or a turntable can only have one vinyl? Or the the vinyl can only have one track?

I couldn't really figure this out from the README, sorry.

@zamith
Copy link

zamith commented Feb 19, 2016

@RuiAAPeres Doing some source diving it seems like a turntable can only have one vinyl. Which leads me to the question, why do you need a turntable at all?

@RuiAAPeres
Copy link
Member Author

@RuiAAPeres Doing some source diving it seems like a turntable can only have one vinyl. Which leads me to the question, why do you need a turntable at all?

@dmcrodrigues ^

@RuiAAPeres
Copy link
Member Author

@zamith although that is currently true, it gives us more space to think about other features, like adding multiple Vinyl to a single Turntable.

@zamith
Copy link

zamith commented Feb 19, 2016

@RuiAAPeres From my experience with something like VCR, there are only two concepts, the cassette or track, which is the actual data, and something that holds the configs, the VCR or what would be the vinyl. I'm unsure of why you would need an extra layer.

You might argue that you want/need to have different configurations in the same app, which I'm not sure is very common. But in that case, you could solve it by having the configs be set on an instance of something and not at the class level, which would make changing the configs a matter of changing the instance you use on a particular test or set of tests.

What things can you foresee as being interesting to add to a turntable? Am I missing something here?

@SandroMachado
Copy link
Contributor

@zamith for instance, two integrations (service A and service B) on the same application. A turntable, two Vinyls (one for the integration with the service A and another one for the service B) and a set of tracks for each Vinyl.

@zamith
Copy link

zamith commented Feb 19, 2016

What do you gain from having two vinyls? You could just as well make the tracks have names, and one vinyl with the tracks for all of the services.

But yeah, if you feel like you need to have different data structures for such a situation, it might make sense. It would definitely be good to have a global config in this situation, though.

@SandroMachado
Copy link
Contributor

IMO, it is much more easier/cleaner to have two different vinyls for each integration. Or for each test case...

@dmcrodrigues
Copy link
Member

@zamith, Turntable is the mechanism that we use to intercept all network traffic that goes through NSURLSession. This interception could be performed using other methods but we definitely need some layer to execute it and perform the corresponding mapping.

I don't think that conceptually a Vinyl should be concerned in doing that, a Vinyl is nothing more than a container of tracks which are the ones that really contain the data. You cannot play that black plastic without a proper device that understands it and can translate his information into music. And the Turntable is that device in our case (actually there's also an internal player involved).

I agree that a turntable should not be restricted to a single vinyl otherwise it may seem redundant, but I really believe that this layer is beneficial to achieve a good separation of concerns.

@zamith
Copy link

zamith commented Feb 19, 2016

@dmcrodrigues Yeah, I get that. Maybe it's the current implementation that led me to question what's the benefit of Vinyl over [Track].

@dmcrodrigues
Copy link
Member

When you go out and want to take your laptop, mouse and headphones normally you take a bag to carry them over! 😛

Basically its more manageable having a Vinyl instead of working directly with [Track], e.g. parsing from disk, but yeah it's only a container.

@RuiAAPeres
Copy link
Member Author

I think we diverged a bit on this issue, so lets back on track (pum not intended).

So, currently we need to create a new Turntable in order to load a Vinyl. There are two ideas on the table:

  • Allow a Turntable, once it's created, to load multiple times a Vinyl.
  • Allow a Global configuration to be setup, so we wouldn't have to replicate a lot of boilerplate every time a new Turntable is created.

These two things might seem orthogonal, but they are related to the same thing: ease of use. When you have a XCTestCase we have the following scenario:

class FooTests: XCTestCase {

  func test_1() {
   // Create Turntable configuration
   // Create turntable with Vinyl + configuration
   // Use the turntable
   }

    func test_2() {
   // Create Turntable configuration
   // Create turntable with Vinyl + configuration
   // Use the turntable
   }
}

Looking at how the lib is working right now, the user could just move the configuration to the XCTestCase scope

class FooTests: XCTestCase {
   // Create Turntable configuration

  func test_1() {
   // Create turntable with Vinyl + configuration
   // Use the turntable
   }

    func test_2() {
   // Create turntable with Vinyl + configuration
   // Use the turntable
   }
}

With the 1st proposal (Turntable allowing multiple Vinyl loads) :

class FooTests: XCTestCase {
   // Create turntable with Configuration

  func test_1() {
   // load Vinyl
   // Use the turntable
   }

    func test_2() {
   // load Vinyl
   // Use the turntable
   }
}

With the 2nd proposal (Global Configuration #33) :

class FooTests: XCTestCase {

  override func setUp() {
   // Setup Global Configuration
   }

  func test_1() {
   // Create turntable with Vinyl
   // Use the turntable
   }

    func test_2() {
   // Create turntable with Vinyl
   // Use the turntable
   }
}

This 2nd proposal is for me the worst, since we have to override the setUp func (see note). The problem I have is not the overriding intrinsically, but that method setting up the Global configuration every time before each test is run.

There is also a 3rd possibility which is implement both the 1st and 2nd proposal, which I am against, since we will just clutter the API and make the Turntable implementation more complex.

Bottom line, I'm in favour of:

  • 1st approach (Turntable loading multiple Vinyl).

Note: It could be another method tearDown or when the XCTestCase is initialized (somewhere else?), the point is: it becomes more cumbersome for the user.

@dmcrodrigues
Copy link
Member

I agree, the 1st approach seems more logical and less cumbersome for the user.

👍

@zamith
Copy link

zamith commented Feb 20, 2016

I agree with the first approach as well. However, is there a setup method
that runs only once per suite? That way you could create the default
turntable there and if need be create a different one (different configs)
in a particular test.
On Sat, 20 Feb 2016 at 10:08, David Rodrigues notifications@github.com
wrote:

I agree, the 1st approach seems more logical and less cumbersome for
the user.

👍


Reply to this email directly or view it on GitHub
#31 (comment).

@dmcrodrigues
Copy link
Member

@zamith yes, XCTestCase has two methods for setup:

  • class func setUp() - Runs once per suite
  • func setUp() - Runs before any test

@SandroMachado
Copy link
Contributor

+1 for the 1st approach.

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

No branches or pull requests

4 participants