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

RUMM-334 Benchmark merged into Integration #63

Merged
merged 3 commits into from
Apr 17, 2020

Conversation

buranmert
Copy link
Contributor

@buranmert buranmert commented Mar 31, 2020

What and why?

We'd like to simplify the repo layout and make it easier to find things for new developers.

How?

  • We removed Integration and Benchmark projects from the repo
  • Those projects are turned into DatadogIntegrationTests in Datadog.xcodeproj

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@buranmert buranmert requested a review from ncreated March 31, 2020 14:55
@buranmert buranmert self-assigned this Mar 31, 2020
@buranmert
Copy link
Contributor Author

Unlike we discussed before, we don't actually need workspaces; for example, we can embed Datadog.xcodeproj into Shopist.xcodeproj without having an extra workspace.

In fact, workspace would make things more complex as embedded projects work as Target Dependencies out of the box (as far as I remember)

@ncreated
Copy link
Member

We can merge Integration and Benchmark in the same xcodeproj as they are almost identical

Good idea 👌. This will also speed up CI by running tests in only one project.

Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

The general idea looks great 👌. However, I think we should not give too much details in CONTRIBUTING.md as we'll have to always keep it up to date. Rather, we may want to dispatch those details to separate README.md files in particular folders.

@buranmert buranmert force-pushed the buranmert/RUMM-334-simplify-repo branch from 6fe0273 to a699311 Compare April 1, 2020 16:09
@ncreated
Copy link
Member

ncreated commented Apr 2, 2020

Unlike we discussed before, we don't actually need workspaces; for example, we can embed Datadog.xcodeproj into Shopist.xcodeproj without having an extra workspace.

In fact, workspace would make things more complex as embedded projects work as Target Dependencies out of the box (as far as I remember)

Whatever works is good for me 👌. The only thing we must guarantee is that Datadog.xcodeproj remains untouched by being embedded somewhere else. We use Datadog.xcodeproj for Carthage.

@buranmert buranmert force-pushed the buranmert/RUMM-334-simplify-repo branch from a699311 to 58504b4 Compare April 6, 2020 15:02
@buranmert buranmert added this to the next-version milestone Apr 7, 2020
@buranmert buranmert force-pushed the buranmert/RUMM-334-simplify-repo branch from 58504b4 to b95e1bd Compare April 14, 2020 15:32
@buranmert buranmert force-pushed the buranmert/RUMM-334-simplify-repo branch 2 times, most recently from 404a72a to 26e9020 Compare April 14, 2020 16:15
@buranmert buranmert marked this pull request as ready for review April 14, 2020 16:30
@buranmert buranmert requested a review from a team as a code owner April 14, 2020 16:30
@buranmert buranmert changed the title RUMM-334 Repo layout simplified RUMM-334 Benchmark merged into Integration Apr 14, 2020
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

We need to also:

  • update paths in sources.swiftlint.yml
  • update paths in tests.swiftlint.yml
  • delete those references from Integration.xcodeproj, as they are invalid:

Screenshot 2020-04-15 at 11 06 24

  • configure all tests to use "Release" configuration - "Debug" doesn't make sense for Benchmarks:

Screenshot 2020-04-15 at 11 11 01

Likewise:

  • let's provision physical devices for running the tests by conditionally importing dependency-manager-tests/xcconfig/app-target.xcconfig in tests-specific-config.xcconfig and resetting all the "Signing" Build Settings to default, so Xcode will use the values from xcconfig.
  • how about rearranging following groups so they are more aligned with the setup? I think we can have three main groups: Benchmark, Integration, Helpers.

Screenshot 2020-04-15 at 11 08 11

```

This will install `swiftlint` and configure custom Datadog file templates for Xcode. Also, `examples-secret.xcconfig` file will be created - update it with a client token obtained on Datadog website.
### Repo structure
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep it up-to-date with a current layout. So maybe we can remove those changes now and add them iteratively with every PR about the layout change, WDYT?

@@ -7,6 +7,11 @@
import Foundation
@testable import Datadog

func clearPersistedLogs() throws {
Copy link
Member

Choose a reason for hiding this comment

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

This function doesn't follow the way we handle Directory and duplicates the code of .delete(). We could basically expose new directory on global constant and leverage already existing delete() implementation:

let logsDirectory = try! Directory(withSubdirectoryPath: LogsPersistenceStrategy.Constants.logFilesSubdirectory)

Then, basically logsDirectory.delete() can be called wherever we need.

This is similar pattern to what we do for:

let temporaryDirectory = obtainUniqueTemporaryDirectory()

@@ -104,33 +103,11 @@ workflows:
inputs:
- scheme: Integration
- simulator_device: iPhone 11
- is_clean_build: 'yes'
- is_clean_build: 'no'
- should_build_before_test: 'no'
Copy link
Member

Choose a reason for hiding this comment

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

Why no clean_build? Does it make any difference on CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that test target doesn't have anything to build, when we call build or even clean it fails with an error along the lines of "this scheme is not configured for running"

with these parameters we run test without explicit build/clean, which works ✅

Copy link
Member

Choose a reason for hiding this comment

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

👍

);
inputPaths = (
);
name = "⚙️ Run linter";
Copy link
Member

Choose a reason for hiding this comment

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

We also need ⚙️ Run linter Build Phase to be re-done in Integration.xcodeproj.

@@ -23,6 +19,8 @@ class BenchmarkTests: XCTestCase {
super.setUp()
if server == nil { server = try! setUpMockServerConnection() }
if serverSession == nil { serverSession = server.obtainUniqueRecordingSession() }

temporaryDirectory.delete()
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't think we need this. Both integration tests and benchmarks do not write to temporaryDirectory 🤔, no?

import HTTPServerMock
import XCTest

class LoggingIntegrationTests: IntegrationTests {
override func setUp() {
super.setUp()
try! clearPersistedLogs()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can do it in base IntegrationTests? This way we won't need @testable import Datadog in this file, and we'll make sure we're accessing only public APIs. WDYT?

@ncreated
Copy link
Member

ncreated commented Apr 15, 2020

  • Also, we need to update Makefile and remove make benchmark.

@buranmert buranmert force-pushed the buranmert/RUMM-334-simplify-repo branch from 26e9020 to d0eb26a Compare April 15, 2020 18:12
@ncreated
Copy link
Member

As suggested in #79 maybe it's worth to consider following for this PR:

  • Update the Integration tests nature to be UI Test target, not unit test target. All the tested logic (import Datadog, initialize SDK and send logs) can be moved from LoggingIntegrationTests to ViewController of an actual app target. The LoggingIntegrationTests will be then simple:
func testWhenRunningAppTarget_allLogsAreUploadedToServer() {
        let app = XCUIApplication()
        app.launch()

        Thread.sleep(...)

         // set of assertions on data delivered to mock server
}

This will very powerful enough to fulfill upcoming needs:

  • testing execution of SDK in app extension (app extensions can be run by UI Test ✅);
  • testing tracing by instrumenting real app callbacks (viewDidLoad, viewDidAppear, navigation between screens, ...);
  • testing auto instrumentation for RUM events (taps, gestures, resource loads).

WDYT @buranmert ?

@buranmert buranmert force-pushed the buranmert/RUMM-334-simplify-repo branch 2 times, most recently from 52c475c to 2214d42 Compare April 16, 2020 12:21
Integration and Benchmark projects are removed
@buranmert buranmert force-pushed the buranmert/RUMM-334-simplify-repo branch from 2214d42 to 5cb2251 Compare April 16, 2020 14:34
@buranmert
Copy link
Contributor Author

@ncreated

  1. provisioning real devices 💯 but let's see when CI adds real device support
  2. turning integration tests into UITests sounds like a good idea but let's talk about this after creating the example project, shall we?

@ncreated
Copy link
Member

@buranmert

turning integration tests into UITests sounds like a good idea but let's talk about this after creating the example project, shall we?

👍 OK, it sounds fine to bring this within example project.

provisioning real devices 💯 but let's see when CI adds real device support

But we also need provisioning to run the integration tests locally 🙌 . Without this, there's no use of benchmark tests and we won't be able to compare the SDK performance in any way. Also, this part of integration test can't be run anywhere:

#if targetEnvironment(simulator)
    // When running on iOS Simulator
    matcher.assertNoValue(forKey: LogMatcher.JSONKey.mobileNetworkCarrierName)
    matcher.assertNoValue(forKey: LogMatcher.JSONKey.mobileNetworkCarrierISOCountryCode)
    matcher.assertNoValue(forKey: LogMatcher.JSONKey.mobileNetworkCarrierRadioTechnology)
    matcher.assertNoValue(forKey: LogMatcher.JSONKey.mobileNetworkCarrierAllowsVoIP)
#else
    // When running on physical device with SIM card registered
    matcher.assertValue(forKeyPath: LogMatcher.JSONKey.mobileNetworkCarrierName, isTypeOf: String.self)
    matcher.assertValue(forKeyPath: LogMatcher.JSONKey.mobileNetworkCarrierISOCountryCode, isTypeOf: String.self)
    matcher.assertValue(forKeyPath: LogMatcher.JSONKey.mobileNetworkCarrierRadioTechnology, isTypeOf: String.self)
    matcher.assertValue(forKeyPath: LogMatcher.JSONKey.mobileNetworkCarrierAllowsVoIP, isTypeOf: Bool.self)
#endif

Before this PR, I used to provision integration and benchmark tests manually. Also the benchmark baseline for my iPhone X was checked into the repo. This file should be re-created for each device we own, so we still have baselines to compare the SDK performance. Now in this PR, it's easy to provide this provisioning, as set of proper *.xcconfigs was configured in #67

Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Few last small comments, the rest looks 💯

CONTRIBUTING.md Outdated

Unit tests are part of the `Datadog.xcodeproj` project. Integration tests and benchmarks are located in separate projects in `instrumented-tests` folder.
Isolated example apps using `cocoapods`, `carthage` and `spm` to ensure SDK is well integrated in all platforms
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Isolated example apps using `cocoapods`, `carthage` and `spm` to ensure SDK is well integrated in all platforms
Isolated example apps using `cocoapods`, `carthage` and `spm` to ensure SDK is well integrated with all supported dependency managers.

@@ -0,0 +1 @@
MOCK_SERVER_ADDRESS=
Copy link
Member

Choose a reason for hiding this comment

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

This file should be git-ignored, otherwise will be producing git changes each time when integration tests are run.

Makefile Outdated
@@ -31,7 +31,7 @@ examples:
@echo "OK 👌"

benchmark:
@cd instrumented-tests/Benchmark && $(MAKE)
@xcodebuild -workspace "Datadog.xcworkspace" -scheme "DatadogIntegrationTests" test -destination "name=iPhone 11 Pro Max"
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 still need make benchmark? This tests are run through bitrise.yml in much nicier way with xcpretty log. Also we duplicate the iPhone 11 Pro Max instrumentation here, I think it's better to centralize it with bitrise.yml only. Now there's IMO no need to do anything for integration tests in Makefile.


import Foundation

func clearPersistedLogs() throws {
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 see any use of this in this PR - shouldn't this be removed 🤔?

}

override func tearDown() {
try! FileManager.default.removeItem(at: directory.url)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just

Suggested change
try! FileManager.default.removeItem(at: directory.url)
self.directory.delete()

now?

@buranmert buranmert force-pushed the buranmert/RUMM-334-simplify-repo branch from 5cb2251 to c25278a Compare April 17, 2020 12:21
@buranmert buranmert force-pushed the buranmert/RUMM-334-simplify-repo branch 2 times, most recently from ca0ba8e to 86e1690 Compare April 17, 2020 13:46
@buranmert buranmert force-pushed the buranmert/RUMM-334-simplify-repo branch from 86e1690 to 76e74c2 Compare April 17, 2020 15:09
@buranmert buranmert requested a review from ncreated April 17, 2020 15:26
@buranmert buranmert merged commit 70c1e70 into master Apr 17, 2020
@buranmert buranmert deleted the buranmert/RUMM-334-simplify-repo branch April 17, 2020 15:56
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.

2 participants