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

moved all tests from src/it to src/test/it #10

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

Conversation

AnshuAg
Copy link
Contributor

@AnshuAg AnshuAg commented May 8, 2024

As part of this issue,

  • I took all the tests from scr/it to src/test/it to make it more readable project structure
  • Removed the basic Sanity Test
  • Removed support of batect and windows and added gitpod support
  • Moving tests from AnyFeatureSpec to AnyFunSuite test

.bsp/sbt.json Outdated Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

Why are we still installing pre-requisites for batect?

Copy link

Choose a reason for hiding this comment

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

Lets also remove this from README local

Additionally, it would be great to include somewhere with a big heading on how to run Test/IntegrationTest

How can we trigger a single test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed pre-requisites for batect and local readme. I have added the command to run a testClass, not able to find one to run single test though. Also since we move all the tests under IT folder, we can run IntegrationTest via sbt test command only

@@ -35,7 +35,7 @@ object CitibikeTransformer {
spark.stop()
}

private def getInputAndOutputPaths(args: Array[String]) = {
private def getInputAndOutputPaths(args: Array[String]): (String, String) = {
Copy link

Choose a reason for hiding this comment

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

Lets simplify this with a Path case class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is still pending, I think you want me to take it out as a class right rather than a function?

Copy link
Contributor

@lauris-tw lauris-tw left a comment

Choose a reason for hiding this comment

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

I have reviewed the non-scala code changes

please wait for atif's review for the scala/test part of the PR.

Overall looks good! Thanks a lot for it: the main differences will be the graphs & the code walkthrough, but I would be happy to add these in another PR :)

Comment on lines 1 to +2
FROM sbtscala/scala-sbt:eclipse-temurin-11.0.15_1.7.1_2.12.16 AS build

#FROM gitpod/workspace-full
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to start from the gitpod base image so we guarantee that all the vscode / intellij things work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense

@@ -10,7 +10,10 @@ RUN wget https://archive.apache.org/dist/spark/spark-3.3.0/spark-3.3.0-bin-hadoo
RUN tar xvf spark-3.3.0-bin-hadoop3.tgz
ENV PATH="/opt/spark-3.3.0-bin-hadoop3/bin:$PATH"

#RUN brew install sbt
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#RUN brew install sbt

@@ -35,7 +35,7 @@ object DailyDriver {
spark.stop()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we split the scala code changes (and testing way from the README.md change? right now this is a bit hard to review what is a code improvement and what is the update of the readme and docker removal)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I tried, but couldn't do it. All my commits were coming in 1 PR since nothing were accepted.

README.md Outdated
target/scala-2.12/tw-pipeline_2.12-0.1.0-SNAPSHOT.jar \
"./output_int" \
./output
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a repeat here for

  > ⚠️ The exercises will be given at the time of interview, and solved by pairing with the interviewer.

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.

3 participants